RFR: 8350177: C2 SuperWord: Integer.numberOfLeadingZeros, numberOfTrailingZeros, reverse and bitCount have input types wrongly turncated for byte and short

[Emanuel Peter]

On Mon, 26 May 2025 07:15:31 GMT, Jasmine Karthikeyan <jkarthikeyan at openjdk.org> wrote:

> Hi all,
> This patch fixes cases in SuperWord when compiling subword types where vectorized code would be given a narrower type than expected, leading to miscompilation due to truncation. This fix is a generalization of the same fix applied for `Integer.reverseBytes` in [JDK-8305324](https://bugs.openjdk.org/browse/JDK-8305324). The patch introduces a check for nodes that are known to tolerate truncation, so that any future cases of subword truncation will avoid creating miscompiled code.
> 
> The patch reuses the existing logic to set the type of the vectors to int, which currently disables vectorization for the affected patterns entirely. Once [JDK-8342095](https://bugs.openjdk.org/browse/JDK-8342095) is merged and automatic casting support is added the autovectorizer should automatically insert casts to and from int, maintaining correctness.
> 
> I've added an IR test that checks for correctly compiled outputs. Thoughts and reviews would be appreciated!

@jaskarth Thanks for looking at this!

How do we know that we have all relevant instructions in the list of `can_subword_truncate`? It seems some shift operations should also be allowed, at least left shift.

I also wonder if there could be ways to truncate long-operations, and we would have to list those as well in `can_subword_truncate`?

I was wondering if we maybe needed a few more tests, given my comments in the `TestShort.java` attached in JBS:
            // While we are at it, we should also have tests for this, even though it currently does not vectorize,

            // but it may in the future and then we have to catch the truncation.
            // out[i] = (short)Long.bitCount(a[i]);

            //out[i] = (short)Integer.rotateLeft(a[i], b[i]);

And just for good measure: should we also add tests for `char`?

src/hotspot/share/opto/superword.cpp line 2496:

> 2494:   int opc = in->Opcode();
> 2495:   return opc == Op_AddI || opc == Op_SubI || opc == Op_MulI || opc == Op_AndI || opc == Op_OrI || opc == Op_XorI
> 2496:     || opc == Op_ReverseBytesS || opc == Op_ReverseBytesUS;

A switch might look nicer here, and be easier to extend later on ;)

src/hotspot/share/opto/superword.cpp line 2553:

> 2551:             const Type* vt = vtn;
> 2552:             int op = in->Opcode();
> 2553:             if (!can_subword_truncate(in)) {

It seems `can_subword_truncate` does not cover `VectorNode::is_shift_opcode`, is that correct? Maybe we are missing IR tests that catch this, scary!

test/hotspot/jtreg/compiler/vectorization/TestSubwordTruncation.java line 64:

> 62: 
> 63:     // Shorts
> 64: 

Suggestion:


Nit: you don't have a similar comment for other types, so just drop it here too ;)

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

PR Review: https://git.openjdk.org/jdk/pull/25440#pullrequestreview-2873934331
PR Comment: https://git.openjdk.org/jdk/pull/25440#issuecomment-2915316621
PR Review Comment: https://git.openjdk.org/jdk/pull/25440#discussion_r2111162309
PR Review Comment: https://git.openjdk.org/jdk/pull/25440#discussion_r2111153772
PR Review Comment: https://git.openjdk.org/jdk/pull/25440#discussion_r2111155491