RFR: 8324641: [IR Framework] Add Setup method to provide custom arguments and set fields [v16]
Christian Hagedorn
chagedorn at openjdk.org
Thu Feb 8 07:52:58 UTC 2024
On Wed, 7 Feb 2024 17:02:35 GMT, Emanuel Peter <epeter at openjdk.org> wrote:
>> **Bigger goal**
>>
>> I am tired of always writing IR tests where I have to write elaborate code in the `@Run` method to create inputs, and then run the test method to create `gold` output values in (hopefully) interpreter mode, and then later verify the results of the compiled method.
>>
>> Hence, I now introduce the "Setup" method, which can create custom argument values.
>> In a later RFE, I will implement automatic result verification, which implicitly intercepts the inputs and outputs of the test method, and compares the behaviour of the interpreter and the compiled code. That way, the pattern will be:
>>
>>
>> @Setup
>> ... specify your arguments and fields ...
>>
>> -> intercept arguments, cache them
>>
>> @Test
>> ... write a test with arbitrary inputs and outputs...
>>
>> -> interpreter mode: intercept outputs and cache them (i.e. gold values)
>> -> compiled mode: intercept outputs and compare them to the gold values.
>>
>> (optional)
>> @Check
>> .. do custom verification...
>>
>>
>> **In this RFE: the Setup Method**
>>
>> The first step is the setup method.
>>
>> Example:
>> https://github.com/openjdk/jdk/blob/08670a5d45b1c45e954e2dc85eb8c92e11e48fb2/test/hotspot/jtreg/testlibrary_tests/ir_framework/examples/SetupExample.java#L49-L87
>
> Emanuel Peter has updated the pull request incrementally with two additional commits since the last revision:
>
> - remove tabs
> - improved and simplified examples
Thanks for adding all these tests! I have some suggestions but only minor things and some more bad test ideas (if I did not miss that you already tested that).
test/hotspot/jtreg/compiler/lib/ir_framework/Arguments.java line 33:
> 31: * One can either specify the individual arguments with values (see {@link Argument}), or use
> 32: * a setup method (see {@link Setup}) to define more complex arguments and/or even set fields values.
> 33: * This annotation can only be applied to a <b>base test</b> or <b>checked test</b>.
Suggestion:
* This annotation can only be applied to a <b>normal test</b>.
test/hotspot/jtreg/compiler/lib/ir_framework/README.md line 44:
> 42: Arguments can be provided with `@Arguments(values = {...})` by providing well-specified inputs for each individual argument. Alternatively, a setup method can be chosen with `@Arguments(setup = "setupMethodName")`, which computes arguments and can also set fields.
> 43:
> 44: More information on normal test methods with a precise definition can be found in the Javadocs of [Test](./Test.java). Concrete examples on how to specify a normal test can be found in [NormalTestsExample](../../../testlibrary_tests/ir_framework/examples/NormalTestExample.java).
Suggestion:
More information on normal test methods with a precise definition can be found in the Javadocs of [Test](./Test.java). Concrete examples on how to specify a normal test can be found in [NormalTestExample](../../../testlibrary_tests/ir_framework/examples/NormalTestExample.java).
test/hotspot/jtreg/compiler/lib/ir_framework/README.md line 54:
> 52: A `@Check(test = "checkMethodName")` annotated method is invoked directly after the `@Test` annotated method `checkMethodName()` is executed. The user can perform various checks, such as test method return value and field value verification.
> 53:
> 54: More information on check methods with a precise definition can be found in the Javadocs of [Check](./Check.java). Concrete examples on how to specify check methods can be found in [CheckedTestsExample](../../../testlibrary_tests/ir_framework/examples/CheckedTestExample.java).
Suggestion:
More information on check methods with a precise definition can be found in the Javadocs of [Check](./Check.java). Concrete examples on how to specify check methods can be found in [CheckedTestExample](../../../testlibrary_tests/ir_framework/examples/CheckedTestExample.java).
test/hotspot/jtreg/compiler/lib/ir_framework/SetupInfo.java line 31:
> 29: * @see Setup
> 30: */
> 31: public record SetupInfo(int invocationCounter) {
I think we could also do this separately once we clean up the `AbstractInfo` class with its subclasses (I don't like how they are structured at the moment) but we should probably also add a `isWarmUp()` method at some point here such that we could use a different set of values for fields/arguments to run the compiled method if one wants to trigger an uncommon path, for example.
test/hotspot/jtreg/compiler/lib/ir_framework/test/ArgumentsProvider.java line 51:
> 49: * @return Returns the arguments to be passed into the test method.
> 50: */
> 51: public abstract Object[] getArguments(Object invocationTarget, int invocationCounter);
Is implied by default:
Suggestion:
Object[] getArguments(Object invocationTarget, int invocationCounter);
test/hotspot/jtreg/compiler/lib/ir_framework/test/ArgumentsProvider.java line 136:
> 134: } catch (Exception e) {
> 135: throw new TestRunException("There was an error while invoking setup method " +
> 136: setupMethod + " on " + target, e);
That was hard to spot as we've discussed offline :-)
test/hotspot/jtreg/compiler/lib/ir_framework/test/TestVM.java line 533:
> 531: "@Setup method cannot be overloaded: " + mOverloaded + " with " + m);
> 532: m.setAccessible(true);
> 533: setupMethodMap.put(m.getName(), m);
You could directly call `put()`. If there was a previous mapping, `put()` will return the old value if there was one or null otherwise.
Suggestion:
Method mOverloaded = setupMethodMap.put(m.getName(), m);
TestFormat.checkNoThrow(mOverloaded == null,
"@Setup method cannot be overloaded: " + mOverloaded + " with " + m);
m.setAccessible(true);
test/hotspot/jtreg/testlibrary_tests/ir_framework/examples/SetupExample.java line 60:
> 58: static Object[] setupLinkedII() {
> 59: int r = RANDOM.nextInt();
> 60: return new Object[]{ r, r + 42};
Suggestion:
return new Object[]{ r, r + 42 };
test/hotspot/jtreg/testlibrary_tests/ir_framework/examples/SetupExample.java line 71:
> 69: @Check(test = "testSetupLinkedII")
> 70: static void checkSetupLinkedII(int res) {
> 71: if (res != 42) { throw new RuntimeException("wrong result " + res); }
Suggestion:
if (res != 42) {
throw new RuntimeException("wrong result " + res);
}
test/hotspot/jtreg/testlibrary_tests/ir_framework/examples/SetupExample.java line 130:
> 128: @Check(test = "testSetupFields")
> 129: static void checkSetupFields(int res) {
> 130: if (res != 42) { throw new RuntimeException("wrong result " + res); }
Suggestion:
if (res != 42) {
throw new RuntimeException("wrong result " + res);
}
test/hotspot/jtreg/testlibrary_tests/ir_framework/examples/SetupExample.java line 142:
> 140: // and never return true. So doing it deterministically can be helpful when we
> 141: // want "low frequency" but a guaranteed "true" at some point.
> 142: return new Object[]{ (long)(cnt % 1_000) };
Any specific reason that this should be a `long` and not an `int` (i.e. also change `testLowProbabilityBranchDeterministic()` to take an int?
test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestBadFormat.java line 655:
> 653: public void badSetupRunAnnotation() {}
> 654:
> 655: // ----------- Ok: Setup Without Test Method -----------------
Maybe change this to "Useless but ok". Maybe we still want to add some verification for these useless methods. But let's do that separately.
test/hotspot/jtreg/testlibrary_tests/ir_framework/tests/TestSetupTests.java line 235:
> 233: @Test
> 234: @Arguments(setup = "setupTooFewArgs")
> 235: public void testTooFewArgs(int a, int b, int c) {}
We should also test:
- Wrongly returning an empty Object array where we expect at least one argument
- Having a `@Setup` method even though the `@Test` method takes no arguments.
But I might have missed that you already provided such tests .
-------------
PR Review: https://git.openjdk.org/jdk/pull/17557#pullrequestreview-1869370604
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482482549
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482483446
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482483791
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482485845
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482491056
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482474887
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482478534
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482479493
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482515708
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482516195
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482517562
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482523158
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482541832
More information about the hotspot-compiler-dev
mailing list