RFR: 8325495: C2: implement optimization for series of Add of unique value [v8]

[Kangcheng Xu]

On Mon, 7 Oct 2024 08:02:35 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   update comments, use explicit opcode comparisons for LShift nodes
>
> src/hotspot/share/opto/addnode.cpp line 567:
> 
>> 565: 
>> 566:     // We can't simply return the lshift node even if ((a << CON) - a) + a cancels out. Ideal() must return a new node.
>> 567:     *multiplier = ((jlong) 1 << con->get_int()) - 1;
> 
> Can't this be an `Identity()` transformation where you can return existing nodes?

Good point. I realized `(x - y) + y => x` is already handled by `AddINode::Identity` and `AddLNode::Identify`. I don't need to repeat here.

> test/hotspot/jtreg/compiler/c2/TestSerialAdditions.java line 64:
> 
>> 62:     private static void addTo2(int a) {
>> 63:         int sum = a + a; // Simple additions like a + a should be kept as-is
>> 64:         verifyResult(a, 2, sum);
> 
> Generally, we should move all verification code out of the `@Test` methods to avoid side effects and worrying about whether the result checking is now compiled or not (we must ensure that the result checking code is interpreted to catch wrong executions with miscompiled code).
> 
> I suggest the following (not tested):
> 
> Introduce a `@Run` method, which is never compiled, for your `@Test` methods. You can still call methods from there but then you should ensure that they are not compiled either with `@DontCompile`:
> 
>     static final Random RANDOM = Utils.getRandomInstance();
> 
>     ...
> 
>     @DontCompile
>     private static void verifyResult(int base, int factor, int observed) { ... }
> 
>     ...
> 
>     @Test
>     @IR(counts = {IRNode.ADD_I, "1", IRNode.LSHIFT_I, "1"})
>     private static int addTo3(int a) {
>         return a + a + a; // a*3 => (a<<1) + a
>     }
> 
>     @Run(test = "addTo3") 
>     void runAddTo3() {
>         int a = RANDOM.nextInt();
>         int result = addTo3(a);
>         verifyResult(a, 3, result);
>     }
> 
> Since the tests are all very similar and require the same setup and verification, you could even go a step further and provide a single shared `@Run` method which is possible:
> 
>     @Test
>     @IR(counts = {IRNode.ADD_I, "1", IRNode.LSHIFT_I, "1"})
>     private static int addTo3(int a) {
>         return a + a + a; // a*3 => (a<<1) + a
>     }
> 
>     @Test
>     @IR(failOn = IRNode.ADD_I)
>     @IR(counts = {IRNode.LSHIFT_I, "1"})
>     private static int addTo4(int a) {
>         return a + a + a + a; // a*4 => a<<2
>     }
> 
>     @Run(test = {"addTo3", "addTo4"}) // List all @Test methods here and make sure you call all of them below.
>     void runTests() {
>         int a = RANDOM.nextInt();
>         verifyResult(a, 3, addTo3(a));
>         verifyResult(a, 4, addTo4(a));
>     }
> 
> 
> This also allows you to run with some more edge case values like `a == 0` or `a == min_int` etc. which gives us even some more confidence.

Thanks for the idea. Converted to custom `@Run` methods and test with `a = 0, 1, min, max, rand`

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20754#discussion_r1790711922
PR Review Comment: https://git.openjdk.org/jdk/pull/20754#discussion_r1790713703