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

[Dean Long]

On Fri, 9 Feb 2024 12:20:53 GMT, Andrew Dinn <adinn at openjdk.org> wrote:

>> 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.

No, sorry for the confusion, that's not what I meant.  I'm OK with the backend using BoolTest values, but if it is going to use Assembler::Condition values, why not use LO/LS/HI/HS like operand cmpOpU does?  To be specific, I'm talking about the interface(COND_INTER)  table.

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

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