RFR: 8349637: Integer.numberOfLeadingZeros outputs incorrectly in certain cases [v3]

[Jasmine Karthikeyan]

On Fri, 14 Feb 2025 15:26:59 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Jasmine Karthikeyan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Improve explanation of logic
>
> Here the reproducer for `Integer.reverse` **truncation** issue.
> 
> 
> public class TestShort {
> 
>     static short[] vals = new short[1024];
>     static short[] results = new short[1024];
>     static short v = (short)0x01234567;
>     static short r = (short)Integer.reverse(v);
> 
>     public static void test() {
>         for (int i = 0; i < 1024; ++i) {
>             //results[i] = (short)Integer.numberOfLeadingZeros(vals[i]);
>             //results[i] = (short)Integer.numberOfTrailingZeros(vals[i]);
>             results[i] = (short)Integer.reverse(vals[i]);
>         }
>    }
> 
>     public static void main(String[] args) {
>         for (int i = 0; i < 1024; ++i) {
>             vals[i] = v;
>         }
>         for (int j = 0; j < 10_000; ++j) {
>             test();
>             for (int i = 0; i < 1024; ++i) {
>                 if (results[i] != r) throw new RuntimeException("Wrong result " + results[i] + " at " + i + " expected " + r);
>             }
>         }
>     }
> }
> 
> 
> 
> java -Xbatch -XX:-TieredCompilation -XX:UseAVX=2 -XX:CompileCommand=compileonly,TestShort::test -XX:+TraceNewVectors TestShort.java 
> CompileCommand: compileonly TestShort.test bool compileonly = true
> TraceNewVectors [AutoVectorization]:  980  LoadVector  === 813 885 864  [[ ]]  @short[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=7; mismatched #vectory<S,16> (does not depend only on test, unknown control)
> TraceNewVectors [AutoVectorization]:  981  ReverseV  === _ 980  [[ ]]  #vectory<S,16>
> TraceNewVectors [AutoVectorization]:  982  StoreVector  === 882 885 866 981  [[ ]]  @short[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=7; mismatched  Memory: @short[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=7;
> Exception in thread "main" java.lang.RuntimeException: Wrong result -6494 at 12 expected 0
> 	at TestShort.main(TestShort.java:48)
> 
> 
> With `-Xint` this test passes.

@eme64 Ah I meant Vector API wasn't impacted for the byte/short cases you mentioned earlier, I sent my comment before I saw your reply about the ints 😅 But it would make sense that the ints are broken, since they emit the same IR. The other operations being impacted by the truncation issue too is a bit worrying. I tested really quickly on an aarch64 device and the cases in your `TestShort.java` broke there too, just to confirm that it's not a backend specific issue.

>From your potential solutions maybe it would be best to temporarily disable the vectorization now to make sure we're not miscompiling, and implement option 2 when we have #23413 by always emitting an `int` vector and letting the autovectorizer fill in the casts. Option 3 likely would be the best performance wise, but would increase the maintenance burden of needing 2 different code paths for the same operation. It could be done later if a need for better performance in this edge case arises.

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

PR Comment: https://git.openjdk.org/jdk/pull/23579#issuecomment-2659688053