RFR: 8240615: is_power_of_2() has Undefined Behaviour and is inconsistent

[Doerr, Martin]

Hi Andrew,

I'm ok with your proposal (regardless of which change gets pushed first), too.

In addition to that we should also fix:
log2_int(int x)
log2_jint(jint x)
which use a wrong type conversion violating
"If x < 0, the function returns 31 on a 32-bit machine and 63 on a 64-bit machine.".

I guess I should open a new issue for that.

Best regards,
Martin


> -----Original Message-----
> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of
> Andrew Haley
> Sent: Montag, 9. März 2020 13:37
> To: Stefan Karlsson <stefan.karlsson at oracle.com>; John Rose
> <john.r.rose at oracle.com>
> Cc: hotspot-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR: 8240615: is_power_of_2() has Undefined Behaviour and is
> inconsistent
> 
> On 3/9/20 12:20 PM, Stefan Karlsson wrote:
> > On 2020-03-09 11:54, Andrew Haley wrote:
> >> So, my plan is to push http://cr.openjdk.java.net/~aph/8240615-1/ and
> >> subsequently patch a few AArch64 cases plus the x86_64 immL_Pow2 and
> >> immL_NotPow2 cases that we already identified.
> >>
> >> I do not intend to do bulk changes to any other clients of
> >> is_power_of_2() because it's often unclear exactly what behaviour is
> >> required, and I'd risk breaking something.
> >>
> >> OK?
> >
> > I would prefer if the x86_64 and aarch64 patches were pushed first, or
> > folded into this patch. Otherwise, we knowingly introduce a regression
> > by pushing the proposed patch above. However, I'll leave it up to the
> > compiler devs/maintainers to decide if this is OK.
> 
> We already have a regression in that AArch64 generates incorrect code.
> IMO it's likely other targets do too.
> 
> However, I'm happy to push the x86_64 and aarch64 patches first or
> as part of this patch.
> 
> --
> Andrew Haley  (he/him)
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> https://keybase.io/andrewhaley
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671