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

[Christian Hagedorn]

On Tue, 17 Aug 2021 00:20:13 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.

First of all, thanks a lot for diving into the various C2 IR transformations and creating IR tests for them! It's valuable to have a test for each of them. Some general points:

- I think you should split the work into the creation of new IR tests of existing transformations (this RFE) and changing actual C2 IR transformations. I suggest to file a new RFE or bug for missing/potential wrong transformations with the corresponding IR tests for them.
- I have only skimmed through the IR tests, yet. I will wait with reviewing them closer in case you decide to transform some of them into custom run tests (see comments in files).
- You should add the `hotspot-compiler` label to the PR as the tests are for C2.

Thanks,
Christian

src/hotspot/share/opto/divnode.cpp line 481:

> 479:   if (op1 == Op_MulI && in1->in(1) == in2) {
> 480:     return new AddINode(in1->in(2), phase->intcon(0));
> 481:   }

I don't think that these transformations are valid due to overflow/underflow. Example:

int x = 1_000_000;
int y = 10_000;
System.out.println((x * y) / x); // Output: 1410 (!= 1_000_000)

You might come up with some special case handling where it applies without an overflow/underflow but I'm not sure if it's worth to do it while not being fragile.

src/hotspot/share/opto/divnode.cpp line 601:

> 599:   if (op1 == Op_MulL && in1->in(1) == in2) {
> 600:     return new AddLNode(in1->in(2), phase->longcon(0));
> 601:   }

Same problem as above.

test/hotspot/jtreg/TEST.groups line 76:

> 74: hotspot_ir_transformations = \
> 75:   ir_transformations
> 76: 

It would be better to move IR tests into a folder within `compiler`. These tests will then always be executed together with the other compiler tests. Then you can remove these lines. 

It might be a good idea in general to collect IR tests in a separate folder to have more control over them or run them with stress flags in the future etc. As you are the first one to add any IR tests with the new framework, we have some freedom to do a choice now where to put such IR tests in the future. I might suggest `compiler/ir_framework` as location, also with the option to add subfolders there, but maybe @iignatev can provide some more insights/better suggestions on this topic.

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

> 45:  * @see IR
> 46:  */
> 47: public class IRNode {

I guess your IDE has applied an alphabetical sorting? I think it's better to keep the old sorting by categories which makes it easier to find specific regexes or to check if a regex is supported or not.

test/hotspot/jtreg/ir_transformations/AddINodeIdealizationTests.java line 23:

> 21:  * questions.
> 22:  */
> 23: package ir_transformations;

Needs to be changed accordingly when updating the test location (see comments above).

test/hotspot/jtreg/ir_transformations/AddINodeIdealizationTests.java line 45:

> 43:     public int simpleZero(int x) {
> 44:         return (x + x) + (x + x);
> 45:     }

This test is named `simpleZero` but the result is `4*x`. Is the method name wrong or is there a mistake in the test itself?

test/hotspot/jtreg/ir_transformations/AddINodeIdealizationTests.java line 53:

> 51:     public int simpleZeroSub(int x) {
> 52:         return (x - x) + (x - x);
> 53:     }

Just an idea: You could also think about transforming some of these base tests into [custom run tests](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/ir_framework/test/CustomRunTest.java) or [checked tests](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/ir_framework/test/CheckedTest.java) (might be enough for simple tests). This could not only verify the correct IR transformations but also that these tests compute the correct results, especially for edge case values (if it makes sense in a particular test). 

A custom run test could look something like this:


    @Test
    @IR(failOn = {IRNode.LOAD, IRNode.STORE, IRNode.MUL, IRNode.DIV, IRNode.SUB})
    // Checks (x - x) => 0 and 0 - 0 => 0
    public int simpleZeroSub(int x) {
        return (x - x) + (x - x);
    }

    @Run(test = "simpleZeroSub")
    public void runSimpleZeroSub() { // Run methods are not compiled and thus run in interpreter
        // This getRandom() call is only supported once JDK-8272567 is in for which a PR is currently open.
        int x = RunInfo.getRandom().nextInt(); // Some random value 
        Asserts.assertEQ(0, simpleZeroSub(x)); // Result check done in interpreter
        x = Integer.MAX_VALUE; // Some edge value
        Asserts.assertEQ(0, simpleZeroSub(x));
        // ...
    }

It might not provide additional value for all your tests but I guess that some of these tests would benefit from additional result checking. I leave it up to you to decide where and if you want to apply these changes.

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

Changes requested by chagedorn (Reviewer).

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