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

[Emanuel Peter]

On Thu, 8 Feb 2024 07:00:55 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - remove tabs
>>  - improved and simplified examples
>
> 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.

We can do that. We will have to see how that plays together with automatic result verification.

> 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 :-)

I'm glad I fixed it!
Thanks for the help when debugging!

> 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);

will do

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

Changing it to int.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482717736
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482716891
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482715616
PR Review Comment: https://git.openjdk.org/jdk/pull/17557#discussion_r1482718022