RFR: 8179004: Add an efficient implementation of the "count trailing zeros" operation
chris.plummer at oracle.com
Fri Apr 21 18:21:05 UTC 2017
On 4/21/17 10:41 AM, Kim Barrett wrote:
>> On Apr 21, 2017, at 1:54 AM, Chris Plummer <chris.plummer at oracle.com> wrote:
>> On 4/19/17 11:17 PM, Kim Barrett wrote:
>>> Please review this addition of the count_trailing_zeros function.
>>> As part of reviewing this change, please feel free to suggest
>>> alternative ways to structure the code. I'm not completely happy with
>>> the way I've done it, but alternatives I've tried seemed worse.
>> Are you talking about the #if/#elseif structure and the ////// delimiters. I'd suggest replacing the ////// with a block comment that stands out. Something like:
>> * GCC Support
> Sure, that makes things stand out a bit more.
> Updated webrev (only comment changes in count_trailing_zeros.hpp)
> What I'm looking for is whether this TARGET_COMPILER dispatch with the
> corresponding code is okay, or whether some other structure would be
> Some alternatives I considered were:
> * Always #include OS_CPU_INCLUDE(count_trailing_zeros). But that
> proliforates files which will be duplicates (possibly duplicates
> by #include of some shared toolchain-related include).
> * Dispatch to #include "utilities/count_trailing_zeros_<tool>.hpp"
> similarly to what globalDefinitions.hpp does. This adds 4 small
> files, which seems a little excessive.
globalDefinitions.hpp seems to be the only other case of using <tool> in
the source. For globalDefinitions.hpp, there is a fair amount of code in
each globalDefinitions_<tool>.hpp header file, so I think in this case
it does help organize the source and adds to readability. In your case
we aren't talking about much code, and it may do more harm than good to
separate the various impls out this way.
> * Put these toolchain-specific definitions in the corresponding
> globalDefintions_<tool>.hpp. But the #include of those files by
> globalDefinitions.hpp happens very early in that file, before things
> like the uintx type are defined. Also, presently debug.hpp can't
> be #include'd by globalDefinitions.hpp, so the latter does similar
> things "by hand". (I think that's something that ought to be fixed.)
I also had this thought before reading your comment above. If your code
was all toolchain independent, would you still consider putting it in
globalDefintions.hpp? If not, I don't necessarily think that it makes
sense to put it there just to piggyback on globalDefinitions.hpp the
<tool> logic. I skimmed through globalDefintions.hpp and couldn't get
enough a good feel for whether or not your API belongs there.
>> Otherwise it looks fine to me, but I'm no expert in this area. The testing looks satisfactory and the bitmap results you are getting are reason enough to add this functionality.
> Thanks. Do you want to be counted as a reviewer?
Sure. FYI I'm not a (R)eviewer.
More information about the hotspot-dev