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

[Christian Hagedorn]

On Thu, 19 Aug 2021 18:14:50 GMT, John Tortugo <github.com+2249648+JohnTortugo at openjdk.org> wrote:

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

Sounds good, thanks!

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

Thanks!

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

Ah I see, it was a little bit confusing :-)

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

Great, thanks! Btw, you can merge and now use `RunInfo.getRandom().XX()` for a handy access to random values (if needed) as the PR for JDK-8272567 was integrated in the meantime.

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

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