RFR: 8267265: Use new IR Test Framework to create tests for C2 IGV transformations [v4]

Fri Sep 17 12:25:53 UTC 2021

On Wed, 1 Sep 2021 00:23:11 GMT, John Tortugo <github.com+2249648+JohnTortugo at openjdk.org> wrote:

>> Hi, can I please get some reviews for this Pull Request? Here is a summary of the changes:
>> 
>> - Add tests, using the new IR-based test framework, for several of the Ideal transformations on Add, Sub, Mul, Div, Loop nodes and some simple Scalar Replacement transformations.
>> - Add more default IR regex's to IR-based test framework.
>> - Changes to Sub, Div and Add Ideal nodes to that transformations on Int and Long types are the whenever possible same.
>> - Changes to Sub*Node, Div*Node and Add*Node Ideal methods to fix some bugs and include new transformations.
>> - New JTREG "ir_transformations" test group under test/hotspot/jtreg.
>
> John Tortugo has updated the pull request incrementally with 146 additional commits since the last revision:
> 
>  - Fix merge mistake.
>  - Merge branch 'jdk-8267265' of https://github.com/JohnTortugo/jdk into jdk-8267265
>  - Addressing PR feedback: move tests to other directory, add custom tests, add tests for other optimizations, rename some tests.
>  - 8273197: ProblemList 2 jtools tests due to JDK-8273187
>    8273198: ProblemList java/lang/instrument/BootClassPath/BootClassPathTest.sh due to JDK-8273188
>    
>    Reviewed-by: naoto
>  - 8262186: Call X509KeyManager.chooseClientAlias once for all key types
>    
>    Reviewed-by: xuelei
>  - 8273186: Remove leftover comment about sparse remembered set in G1 HeapRegionRemSet
>    
>    Reviewed-by: ayang
>  - 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302
>    
>    Reviewed-by: jiefu, serb
>  - 8273092: Sort classlist in JDK image
>    
>    Reviewed-by: redestad, ihse, dfuchs
>  - 8273144: Remove unused top level "Sample Collection Set Candidates" logging
>    
>    Reviewed-by: iwalulya, ayang
>  - 8262095: NPE in Flow$FlowAnalyzer.visitApply: Cannot invoke getThrownTypes because tree.meth.type is null
>    
>    Co-authored-by: Jan Lahoda <jlahoda at openjdk.org>
>    Co-authored-by: Vicente Romero <vromero at openjdk.org>
>    Reviewed-by: jlahoda
>  - ... and 136 more: https://git.openjdk.java.net/jdk/compare/ac430bf7...463102e2

Thanks for your effort to write tests for all these different kinds of transformations! Generally, they look good and are worth to have! You should add `@bug 8267265` to all files.

test/hotspot/jtreg/compiler/c2/irTests/AddINodeIdealizationTests.java line 34:

> 32:  * @run driver compiler.c2.irTests.AddINodeIdealizationTests
> 33:  */
> 34: public class AddINodeIdealizationTests {

General comments, also applies to the other test files:
It might be good to sanity check the output results of all these transformations (even though they are simple). Since the tests only use simple randomized ints, you could use a single `@Run` method instead of one for each test. This could look something like [this](https://gist.github.com/chhagedorn/b16aba260a8fcf27c082beccf2cec0a3).

test/hotspot/jtreg/compiler/c2/irTests/AddINodeIdealizationTests.java line 40:

> 38: 
> 39:     @Test
> 40:     @IR(failOn = {IRNode.LOAD, IRNode.STORE, IRNode.MUL, IRNode.DIV, IRNode.SUB})

In this test and all the following ones (including the other files), I think you can remove unrelated `failOn` regexes on operations that are not part of the test. For example, in this test you can safely remove `IRNode.MUL, DIV, and SUB`.

test/hotspot/jtreg/compiler/c2/irTests/AddINodeIdealizationTests.java line 91:

> 89:     @IR(failOn = {IRNode.LOAD, IRNode.STORE, IRNode.MUL, IRNode.DIV, IRNode.SUB})
> 90:     @IR(counts = {IRNode.ADD, "2"})
> 91:     // Checks (x + c1) + y => (x + y) + c1

Unfortunately, a limitation of the framework to check the correct inputs of IR nodes.

test/hotspot/jtreg/compiler/c2/irTests/AddLNodeIdealizationTests.java line 151:

> 149:         return (a - b) + (c - a);
> 150:     }
> 151: 

Compared to the `AddI` tests, you've missed the case `(a - b) + (b - c) => (a - c)` here.

test/hotspot/jtreg/compiler/c2/irTests/DivINodeIdealizationTests.java line 44:

> 42:     // Checks x / x => 1
> 43:     public int constant(int x) {
> 44:         return x / x;

This fails when `x` is zero with an `ArithmeticException`. I suggest to convert this into a custom run test and catch this case - maybe also testing zero as separate case to see if an exception is thrown with compiled code.

test/hotspot/jtreg/compiler/c2/irTests/DivINodeIdealizationTests.java line 68:

> 66:     // Checks x / (y / y) => x
> 67:     public int identityThird(int x, int y) {
> 68:         return x / (y / y);

Same problem as above with `y = 0`.

test/hotspot/jtreg/compiler/c2/irTests/DivINodeIdealizationTests.java line 79:

> 77:     // Hotspot should keep the division because it may cause a division by zero trap
> 78:     public int retainDenominator(int x, int y) {
> 79:         return (x * y) / y;

Same problem as above with `y = 0`.

test/hotspot/jtreg/compiler/c2/irTests/DivLNodeIdealizationTests.java line 34:

> 32:  * @run driver compiler.c2.irTests.DivLNodeIdealizationTests
> 33:  */
> 34: public class DivLNodeIdealizationTests {

Same div by zero problems as with `DivI`. Should be adjusted analogously.

test/hotspot/jtreg/compiler/c2/irTests/MulINodeIdealizationTests.java line 45:

> 43:     //Checks Max(a,b) * min(a,b) => a*b
> 44:     public int excludeMaxMin(int x, int y){
> 45:         return Math.max(x, y) * Math.min(x, y);

`Math.min/max()` is intrinsified and HotSpot generates `CMove` nodes (see `LibraryCallKit::generate_min_max()`) for them. But it looks like `MulNode::Ideal` misses this check for `CMove` nodes. That could be done in a separate RFE (and then this test could be improved to check if the `CMove` node was removed). 

Anyways, min/max nodes are mainly used for loop limit computations, so it's harder to test this transformation in an easy way.

test/hotspot/jtreg/compiler/c2/irTests/MulLNodeIdealizationTests.java line 43:

> 41:     @IR(failOn = {IRNode.LOAD, IRNode.STORE, IRNode.DIV, IRNode.CALL})
> 42:     @IR(counts = {IRNode.MUL, "1"})
> 43:     //Checks Max(a,b) * min(a,b) => a*b

See comments for `MulI`.

test/hotspot/jtreg/compiler/c2/irTests/SubINodeIdealizationTests.java line 164:

> 162:     @Arguments(Argument.RANDOM_EACH)
> 163:     @IR(failOn = {IRNode.LOAD, IRNode.STORE, IRNode.MUL, IRNode.DIV, IRNode.SUB, IRNode.ADD})
> 164:     // Checks 0 - (a >> 31) => a >> 31

Comment should be adjusted to differentiate between signed and unsigned shifts. And a rule should be added to check that the `RShiftI` node was converted into an `URShiftI` node.

test/hotspot/jtreg/compiler/c2/irTests/SubLNodeIdealizationTests.java line 155:

> 153:     @Arguments(Argument.RANDOM_EACH)
> 154:     @IR(failOn = {IRNode.LOAD, IRNode.STORE, IRNode.MUL, IRNode.DIV, IRNode.SUB, IRNode.ADD})
> 155:     // Checks 0 - (a >> 63) => a >>> 63

Same as for `SubI` above, a rule should be added for the shift nodes.

test/hotspot/jtreg/compiler/c2/irTests/loopOpts/LoopIdealizationTests.java line 43:

> 41:     @Test
> 42:     @IR(failOn = {IRNode.LOAD, IRNode.STORE, IRNode.MUL, IRNode.DIV, IRNode.ADD, IRNode.SUB, IRNode.LOOP, IRNode.COUNTEDLOOP, IRNode.COUNTEDLOOP_MAIN, IRNode.CALL})
> 43:     //Checks that a for loop with 0 iterations is removed

Missing space after `//` and also for other comments below.

test/hotspot/jtreg/compiler/c2/irTests/loopOpts/LoopIdealizationTests.java line 44:

> 42:     @IR(failOn = {IRNode.LOAD, IRNode.STORE, IRNode.MUL, IRNode.DIV, IRNode.ADD, IRNode.SUB, IRNode.LOOP, IRNode.COUNTEDLOOP, IRNode.COUNTEDLOOP_MAIN, IRNode.CALL})
> 43:     //Checks that a for loop with 0 iterations is removed
> 44:     public void zeroIterForLoop(){

Missing space between `)` and `{` and also on other lines below.

test/hotspot/jtreg/compiler/c2/irTests/loopOpts/LoopIdealizationTests.java line 52:

> 50:     @Test
> 51:     @IR(failOn = {IRNode.LOAD, IRNode.STORE, IRNode.MUL, IRNode.DIV, IRNode.ADD, IRNode.SUB, IRNode.LOOP, IRNode.COUNTEDLOOP, IRNode.COUNTEDLOOP_MAIN, IRNode.CALL})
> 52:     //Checks that a for loop with 0 iterations is removed

Actually there is 1 iteration but we break it immediately (i.e. the loop is entered).

test/hotspot/jtreg/compiler/c2/irTests/loopOpts/LoopIdealizationTests.java line 89:

> 87:             if (i == 0){
> 88:                 break;
> 89:            }else{

Spaces around `else`.

test/hotspot/jtreg/compiler/c2/irTests/loopOpts/LoopIdealizationTests.java line 141:

> 139:     //Checks that a while loop with 1 iteration is simplified to straight code
> 140:     public void oneIterDoWhileLoop(){
> 141:         do{

Spacing

test/hotspot/jtreg/compiler/c2/irTests/scalarReplacement/ScalarReplacementTests.java line 29:

> 27: /*
> 28:  * @test
> 29:  * @summary Tests that Escape Analisys and Scalar Replacement is able to handle some simple cases.

Typo: Analisys -> Analysis

test/hotspot/jtreg/compiler/c2/irTests/scalarReplacement/ScalarReplacementTests.java line 33:

> 31:  * @run driver compiler.c2.irTests.scalarReplacement.ScalarReplacementTests
> 32:  */
> 33: public class ScalarReplacementTests {

You should also add some rules to check if there is an allocation or not.

test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java line 162:

> 160:     public static final String SCOPE_OBJECT = "(.*# ScObj.*" + END;
> 161:     public static final String MEMBAR = START + "MemBar" + MID + END;
> 162: 

I suggest to move all newly added regex together here.

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

Changes requested by chagedorn (Reviewer).

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