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

[John Tortugo]

On Tue, 17 Aug 2021 08:01:01 GMT, Christian Hagedorn <chagedorn 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.
>
> 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 (!= 10_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.

Oops! Thanks for catching that. I'll remove it and the test.

> 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.

Sounds good to me!

> 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?

I think the "Zero" suffix was actually an index, as in "simple-0", "simple-1", etc. Clearly not a good name in this case. I'll rename it. Thanks.

> 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.

Thank you @chhagedorn , I think this is a good idea. I'll follow your suggestion and transform some tests into `custom run tests`.

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

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