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

[Emanuel Peter]

On Mon, 16 Jun 2025 05:32:19 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!
>
> Jasmine Karthikeyan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add assert for unexpected node in truncation

Thanks for the updates @jaskarth !

I have some more minor suggestions now.

Testing was all good, but we'll have to re-run it with all the new asserts once you have addressed my suggestions :)

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

> 2514:   case Op_XorI:
> 2515:     return true;
> 2516:   }

Suggestion:

  // Can be truncated:
  switch (opc) {
  case Op_AddI:
  case Op_SubI:
  case Op_MulI:
  case Op_AndI:
  case Op_OrI:
  case Op_XorI:
    return true;
  }

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

> 2519:   if (VectorNode::is_shift_opcode(opc)) {
> 2520:     return false;
> 2521:   }

Is right shift also not truncatable? Can you add a comment why?

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

> 2534:   case Op_CountLeadingZerosI:
> 2535:   case Op_CountTrailingZerosI:
> 2536:     break;

Suggestion:

  switch (opc) {
  // Cannot be truncated:
  case Op_AbsI:
  case Op_DivI:
  case Op_MinI:
  case Op_MaxI:
  case Op_CMoveI:
  case Op_RotateRight:
  case Op_RotateLeft:
  case Op_PopCountI:
  case Op_ReverseBytesI:
  case Op_ReverseI:
  case Op_CountLeadingZerosI:
  case Op_CountTrailingZerosI:
    return false;

Here you did a break in assert. For consistency, let's make it a return.

I would also understand if you did not want to have any return in the assert code, to make sure product and debug do not have different behavior. Up to you.

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

> 2536:     break;
> 2537:   default:
> 2538:     assert(false, "Unexpected node: %s", NodeClassNames[in->Opcode()]);

Suggestion:

    // If this assert it hit, that means that we need to determine if the node can be safely truncated,
    // and then add it to the list of truncatable nodes or the list of non-truncatable ones just above.
    // In product, we just return false, which is always correct.
    assert(false, "Unexpected node: %s", NodeClassNames[in->Opcode()]);

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25440#pullrequestreview-2930728433
PR Review Comment: https://git.openjdk.org/jdk/pull/25440#discussion_r2149105906
PR Review Comment: https://git.openjdk.org/jdk/pull/25440#discussion_r2149107187
PR Review Comment: https://git.openjdk.org/jdk/pull/25440#discussion_r2149108667
PR Review Comment: https://git.openjdk.org/jdk/pull/25440#discussion_r2149114689