RFR: 8276162: Optimise unsigned comparison pattern [v3]

[Tobias Hartmann]

On Thu, 11 Nov 2021 05:12:09 GMT, Mai Đặng Quân Anh <duke at openjdk.java.net> wrote:

>> This patch changes operations in the form `x +- Integer.MIN_VALUE <=> y +- Integer.MIN_VALUE`, which is a pattern used to do unsigned comparisons, into `x u<=> y`.
>> 
>> In addition to being basic operations, they may be utilised to implement range checks such as the methods in `jdk.internal.util.Preconditions`, or in places where the compiler cannot deduce the non-negativeness of the bound as in `java.util.ArrayList`.
>> 
>> Thank you very much.
>
> Mai Đặng Quân Anh has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - replace cmpx->Opcode() with cmpx_op
>  - address reviews, remove checks for subtraction operatios

Given that there is `Integer/Long.compareUnsigned` using this idiom, it seems reasonable to optimize. Some general comments:
- Your benchmark does not cover all the cases you are optimizing. Maybe you should also add the `Integer.compareUnsigned` variants.
- You need a correctness test as well, ideally using the IR verification framework to also verify that the optimizations are actually performed.

src/hotspot/share/opto/subnode.cpp line 1534:

> 1532:   }
> 1533: 
> 1534:   // Change x + Integer.MIN_VALUE <=> y + Integer.MIN_VALUE into x u<=> y

The `<=>` in the comment is confusing because it usually denotes logical equality. Also, you are only handling `<` and `>` below. What about the other variants? Shouldn't they be canonicalized in `idealize_test` (see `ifnode.cpp`)?

I would recommend making it explicit in the comment and use brackets for readability:

// Change (x + Integer.MIN_VALUE < y + Integer.MIN_VALUE) into (x u< y) and
//        (x + Integer.MIN_VALUE > y + Integer.MIN_VALUE) into (x u> y).

src/hotspot/share/opto/subnode.cpp line 1546:

> 1544:     } else if (cmp2_op == Op_AddI &&
> 1545:                phase->type(cmp2->in(2)) == TypeInt::MIN) {
> 1546:       Node *ncmp = phase->transform(new CmpUNode(cmp1->in(1), cmp2->in(1)));

`Node *ncmp` -> `Node* ncmp`

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

Changes requested by thartmann (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6101