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

[Doerr, Martin]

Sorry, my comment about log2_int + log2_jint was not precise.

The comment
"If x < 0, the function returns 31 on a 32-bit machine and 63 on a 64-bit machine."
belongs to log2_intptr, but it also applies to log2_int + log2_jint which is not desirable IMHO.
I guess using zero extend would be better for these 2 functions: "If x < 0, the function returns 31 on all machines."

Best regards,
Martin


> -----Original Message-----
> From: hotspot-runtime-dev <hotspot-runtime-dev-
> bounces at openjdk.java.net> On Behalf Of Doerr, Martin
> Sent: Montag, 9. März 2020 15:34
> To: Andrew Haley <aph at redhat.com>; 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
> 
> 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