RFR: 8179004: Add an efficient implementation of the "count trailing zeros" operation

[Chris Plummer]

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)
> http://cr.openjdk.java.net/~kbarrett/8179004/hotspot.01/
>
> What I'm looking for is whether this TARGET_COMPILER dispatch with the
> corresponding code is okay, or whether some other structure would be
> preferred.
>
> 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.

Chris
>