RFR: 8324641: [IR Framework] Add Setup method to provide custom arguments and set fields

Christian Hagedorn chagedorn at openjdk.org
Mon Feb 5 12:57:06 UTC 2024 

On Wed, 24 Jan 2024 14:39:39 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

That's a nice feature! I have a few comments.

As already discussed offline, I think we should clean up the entire code in the test VM and group verification code together to avoid spreading it all over the place. But this should not block this PR and should be done separately.

test/hotspot/jtreg/compiler/lib/ir_framework/Arguments.java line 30:

> 28: 
> 29: /**
> 30:  * This annotation is used for test methods (see {@link Test}), to specify what values should be passed as arguments.

Suggestion:

 * This annotation is used for test methods (see {@link Test}) to specify what values should be passed as arguments.

test/hotspot/jtreg/compiler/lib/ir_framework/Arguments.java line 31:

> 29: /**
> 30:  * This annotation is used for test methods (see {@link Test}), to specify what values should be passed as arguments.
> 31:  * One can either specify the individual arguments with values (see {@link Argument}). Alternatively, one can specify

Suggestion:

 * One can either specify the individual arguments with values (see {@link Argument}), or use

test/hotspot/jtreg/compiler/lib/ir_framework/Arguments.java line 32:

> 30:  * This annotation is used for test methods (see {@link Test}), to specify what values should be passed as arguments.
> 31:  * One can either specify the individual arguments with values (see {@link Argument}). Alternatively, one can specify
> 32:  * a setup method (see {@link Setup}), which can compute arbitrary arguments, and can even be used to set fields.

Suggestion:

 * a setup method (see {@link Setup}) to define more complex arguments and/or even set fields values.

test/hotspot/jtreg/compiler/lib/ir_framework/Arguments.java line 48:

> 46:      * Get the setup method name.
> 47:      */
> 48:     String[] setup() default {};

Should be a unique setup method. So, you can use `String` instead of `String[]`.
Suggestion:

    String setup() default {};

test/hotspot/jtreg/compiler/lib/ir_framework/README.md line 36:

> 34: The framework offers various annotations and flags to control how your test code should be invoked and being checked. This section gives an overview over all these features.
> 35: 
> 36: ### 2.1 Test Method

I suggest to leave this as "Different Tests" and go with the two kinds "normal test" vs "custom run test". A test method itself is just the method annotated with `@Test` which could be used with both kinds of tests.

test/hotspot/jtreg/compiler/lib/ir_framework/README.md line 52:

> 50: More information on checked tests with a precise definition can be found in the Javadocs of [Run](./Run.java). Concrete examples on how to specify a custom run test can be found in [CustomRunTestsExample](../../../testlibrary_tests/ir_framework/examples/CustomRunTestExample.java).
> 51: 
> 52: ### 2.2 Setup Method

I think both 2.2 and 2.3 should go under normal test as they cannot be used in custom run tests.

test/hotspot/jtreg/compiler/lib/ir_framework/README.md line 53:

> 51: 
> 52: ### 2.2 Setup Method
> 53: A `@Setup` annotated method can provide custom arguments and set fields before a normal test is run. A normal test method can be annotated with `@Arguments(setup = {"setupMethodName"})` to specify which setup method is to be used.

See comment in `@Arguments` about `String` vs `String[]`, we can then directly use (same at other places): 

@Arguments(setup = "setupMethodName")

test/hotspot/jtreg/compiler/lib/ir_framework/Setup.java line 31:

> 29: /**
> 30:  * This annotation is used to identify Setup methods. These can be used to compute arbitrary arguments for a test
> 31:  * method (see {@link Test}), as well as set fields. A test method can use a Setup method, by specifying it in a

Suggestion:

 * method (see {@link Test}), as well as to set field values. A test method can use a setup method, by specifying it in a

test/hotspot/jtreg/compiler/lib/ir_framework/Setup.java line 33:

> 31:  * method (see {@link Test}), as well as set fields. A test method can use a Setup method, by specifying it in a
> 32:  * {@link Arguments} annotation. A setup method can optionally take a {@link SetupInfo} as an argument. The for the
> 33:  * arguments for the test methods are returned in an Object[].

Suggestion:

 * {@link Arguments} annotation. A setup method can optionally take a {@link SetupInfo} as an argument. The
 * arguments for the test methods are returned as a new object array.

test/hotspot/jtreg/compiler/lib/ir_framework/Setup.java line 34:

> 32:  * {@link Arguments} annotation. A setup method can optionally take a {@link SetupInfo} as an argument. The for the
> 33:  * arguments for the test methods are returned in an Object[].
> 34:  *

Maybe also add a hint how to use it similar to what's found at `@Test`:


Examples on how to use test methods can be found in {@link ir_framework.examples.SetupExample} and also as part of the internal testing in the package {@link ir_framework.tests}.

test/hotspot/jtreg/compiler/lib/ir_framework/SetupInfo.java line 31:

> 29:  * @see Setup
> 30:  */
> 31: public class SetupInfo {

I think you could use a record for that one.

test/hotspot/jtreg/compiler/lib/ir_framework/test/ArgumentValue.java line 249:

> 247:     }
> 248: 
> 249:     public Object getValue(int invocationCounter) {

`invocationCounter` is unused and can be removed - same in sub class.

test/hotspot/jtreg/compiler/lib/ir_framework/test/ArgumentsProvider.java line 76:

> 74:         if (setup.length > 0) {
> 75:             TestFormat.check(setup.length == 1,
> 76:                              "@Arguments: setup should specify exactly one setup method " +

Suggestion:

                             "@Arguments: "setup" should specify exactly one @Setup method " +

test/hotspot/jtreg/compiler/lib/ir_framework/test/ArgumentsProvider.java line 79:

> 77:                              " but got " + setup.length + " in " + method);
> 78:             TestFormat.check(values.length == 0,
> 79:                              "@Arguments: specify only one of setup and values in " + method);

Suggestion:

                             "@Arguments: Can only specify "setup" or "values" but not both in " + method);

test/hotspot/jtreg/compiler/lib/ir_framework/test/ArgumentsProvider.java line 89:

> 87:             TestFormat.check(values.length > 0,
> 88:                              "@Arguments: empty annotation not allowed, need to have at least one values " +
> 89:                              "argument, or specify a setup method. In method " + method);

Suggestion:

                             "@Arguments: Empty annotation not allowed. Either specify "values" or "setup" in " + method);

test/hotspot/jtreg/compiler/lib/ir_framework/test/TestVM.java line 508:

> 506: 
> 507:     /**
> 508:      *  Collect all @Setup annodated methods and add them to setupMethodMap, for conveninence to reference later from

Suggestion:

     *  Collect all @Setup annotated methods and add them to setupMethodMap, for convenience to reference later from

test/hotspot/jtreg/compiler/lib/ir_framework/test/TestVM.java line 762:

> 760:                          "Cannot use @Test " + testMethod + " for more than one @Run/@Check method. Found: "
> 761:                          + m + ", " + attachedMethod);
> 762:         TestFormat.check(test.hasDefaultArgumentsProvider(),

`hasDefaultArgumentsProvider()` -> `argumentsProvider.isDefault()` is only used here and uses an `instanceof` check in the super class with a sub class. This does not seem right. I suggest to do this entire format check over the `setupTestMap` instead:

Suggestion:

        TestFormat.check(!setupMethodMap.containsKey(testMethod.getName()),

This would also allow us to convert `ArgumentsProvider` into an interface (if we move `getArgumentsProvider()` out or to a separate `ArgumentsProviderBuilder` class).

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

PR Review: https://git.openjdk.org/jdk/pull/17557#pullrequestreview-1862566955
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478142325
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478143137
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478148661
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478138754
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478164802
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478172688
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478175547
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478093548
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478095045
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478099231
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478101164
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478178515
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478108834
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478111335
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478116014
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478104540
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1478129870

More information about the hotspot-compiler-dev mailing list