RFR: 8321308: AArch64: Fix matching predication for cbz/cbnz

[Andrew Dinn]

On Thu, 8 Feb 2024 22:45:27 GMT, Dean Long <dlong at openjdk.org> wrote:

>> src/hotspot/cpu/aarch64/aarch64.ad line 16188:
>> 
>>> 16186:     Label* L = $labl$$label;
>>> 16187:     Assembler::Condition cond = (Assembler::Condition)$cmp$$cmpcode;
>>> 16188:     if (cond == Assembler::EQ || cond == Assembler::LE)
>> 
>> So this is an explicitly unsigned comparison `CmpU` yet it has been converted into an explicitly signed `Assembler::LE`? Is that right?
>
> I agree, this is confusing.  Why not fix cmpOpUEqNeLeGt so that it uses the unsigned condition code values?

@dean-long I am not sure what you mean by 'so that it uses the unsigned condition code values'. Are you suggesting that it would be inappropriate for a `CmpU` node to employ an `le` condition? That is certainly a possibility that arises with the current opto code.

n.b. Field '$cmp$$cmpcode' of the CmpUNode is actually a value of the enum type `BoolTest`. So, strictly the cast on line 16187 ought to perform the conversion using

    Assembler::Condition cond = to_assembler_cond(cmp$$cmpcode);

but the result will be the same since the values in enum `Assembler::Condition` mirror those in enum `BoolTest`.

In this specific case the compiler installs `BoolTest::gt` into the `CmpUNode` during bytecode parsing because that is the test specified in the example code provided by @fg1417. This condition will be flipped to BoolTest::le as part of normalization of the resulting IfNode (the true and false branches attached to the if are switched as part of this change). So, after parsing and normalization we end up with a node that looks like `CmpU[le](LoadRange, IntConstant(0))` and this is what is embedded as an operand of the `If` node that gets considered as a match candidate by the current rule.

So, while there is nothing to stop an unsigned comparison passed through to the back end from employing a `BoolTest::le` comparison against 0, by contrast the compiler should never pass through a `BoolTest::lt` unsigned comparison against 0 because it will always evaluate to false. I am not certain but I believe from my brief look at the code that this case gets detected as part of the IfNode normalization and the if is eliminated as a dead node.

Granted then that the back end can see `CmpU[le](op1, IntConstant(0))`, the current operand and rule do not cover that case. Instead of `LE` they actually test for an `LT` against 0 -- which we are never going to match. If we change the rule to match for `LE` then it *will* match and insert an appropriate compare and branch. With the current match failure the `CmpU` and `If` get translated independently leading to the inefficiency @fg1417 describes.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16989#discussion_r1484249573