RFR: 8273454: C2: Transform (-a)*(-b) into a*b [v4]

[Tobias Hartmann]

On Thu, 9 Sep 2021 15:02:32 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

>> The transformation reduce instructions in generated code.
>> 
>> ### x86_64:
>> 
>> Before:
>> ```  
>>   0x00007fb92c78b3ac:   neg    %esi
>>   0x00007fb92c78b3ae:   neg    %edx
>>   0x00007fb92c78b3b0:   mov    %esi,%eax
>>   0x00007fb92c78b3b2:   imul   %edx,%eax                    ;*imul {reexecute=0 rethrow=0 return_oop=0}
>>                                                             ; - TestSub::runSub at 4 (line 9)
>> 
>> After:
>> 
>>                                                            ; - TestSub::runSub at -1 (line 9)
>>   0x00007fc8c05b74ac:   mov    %esi,%eax
>>   0x00007fc8c05b74ae:   imul   %edx,%eax                    ;*imul {reexecute=0 rethrow=0 return_oop=0}
>>                                                             ; - TestSub::runSub at 4 (line 9)
>> 
>> 
>> 
>> ### AArch64:
>> Before:
>> 
>>  0x0000ffff814b4a70:   neg     w11, w1
>>  0x0000ffff814b4a74:   mneg    w0, w2, w11                 ;*imul {reexecute=0 rethrow=0 return_oop=0}
>>                                                             ; - TestSub::runSub at 4 (line 9)
>> 
>> 
>> After:
>> 
>>  0x0000ffff794a67f0:   mul     w0, w1, w2                  ;*imul {reexecute=0 rethrow=0 return_oop=0}
>>                                                             ; - TestSub::runSub at 4 (line 9)
>
> Zhengyu Gu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix node in place instead of creating new node

Changes requested by thartmann (Reviewer).

src/hotspot/share/opto/mulnode.cpp line 70:

> 68:   Node *in2 = in(2);
> 69:   if (in1->is_Sub() && in2->is_Sub()) {
> 70:     Node* n11 = in1->in(1);

For consistency with below code, I would name the local `in11` or simply use `phase->type(in1->in(1))` because it's the only user.

test/hotspot/jtreg/compiler/integerArithmetic/TestNegMultiply.java line 26:

> 24: /**
> 25:  * @test
> 26:  * @bug 8273454

The test needs `* @key randomness`

test/hotspot/jtreg/compiler/integerArithmetic/TestNegMultiply.java line 36:

> 34: 
> 35: public class TestNegMultiply {
> 36:     private static Random random = new Random();

You should use `Utils.getRandomInstance()` from `jdk.test.lib.Utils` to ensure that the seed is printed for reproducibility. You can check other tests for an example.

test/hotspot/jtreg/compiler/integerArithmetic/TestNegMultiply.java line 45:

> 43:     private static void testInt(int a, int b) {
> 44:         int expected = (-a) * (-b);
> 45:         for (int i = 0; i < 20_000; i++) {

Why do you need a second loop in here? It's sufficient to set `TEST_COUNT` high enough to trigger compilation. I would suggest something like this:


private static int testInt(int a, int b) {
    return (-a) * (-b);
}

private static void runIntTests() {
    for (int i = 0; i < TEST_COUNT; i++) {
        int a = random.nextInt();
        int b = random.nextInt();
        int res = testInt(a, b);
        Asserts.assertEQ(a * b, res);
    }
}


And then run with `-XX:CompileCommand=dontinline,TestNegMultiply::test*`. No need to disable OnStackReplacement.

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

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