RFR: 8282365: Consolidate and improve division by constant idealizations [v35]

Quan Anh Mai qamai at openjdk.org
Tue Dec 12 16:09:06 UTC 2023


On Thu, 7 Dec 2023 22:26:54 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   missing include
>
> src/hotspot/share/utilities/globalDefinitions.hpp line 1107:
> 
>> 1105:   using U = std::make_unsigned_t<T>;
>> 1106:   return (x >= 0) ? x : U(0) - U(x);
>> 1107: }
> 
> I understand what this to change to ABS is doing, though it's not obvious.  (Dodging overflow UB
> for -x when x is the minimum value of a signed integral type.)  I'm not entirely sure that's a wise move.
> 
> As written this will trigger `-Wconversion` warnings someday (maybe).  static_casting the subtraction
> result to T will eliminate that concern.
> 
> However, this is an API change.  The previous definition worked for floating point types, while this
> change does not.  (std::make_unsigned requires T be an integral or enum, but not bool, type.)
> 
> I also don't understand why this change is part of this PR.
> 
> So I'm inclined to say no to this change without some compelling rationale.

Sure I have reverted that also.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/9947#discussion_r1424233574


More information about the hotspot-compiler-dev mailing list