RFR: 8342387: C2 SuperWord: refactor and improve compiler/loopopts/superword/TestDependencyOffsets.java [v2]

Wed Oct 23 06:00:15 UTC 2024

On Tue, 22 Oct 2024 13:08:49 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> I want to refactor `TestDependencyOffsets.java` using the `CompileFramework`.
>> 
>> Reasons:
>> - I soon need to modify the IR rules in this test soon anyway (https://github.com/openjdk/jdk/pull/21521), and so a refactor might be good to do first.
>> - The generator script used to be a `generator.py`, stored not on `git` but `JBS`. Not great. Now we have it in Java code, and maintenance is easier.
>> - Since I first wrote that test, I have now introduced the `IRNode.VECTOR_SIZE`. This allows:
>>   - Simplifying the logic for the IR rules (removed the `IRRange` and `IRBool`, and the `Platform`).
>>   - Strengthening the rules.
>> - I was able to add `random` offsets. This means we have better coverage, and do not rely on just hand-crafted values.
>> 
>> I extensively use `String.format` and `StringBuilder`... would be nicer to have string-templates but they don't exist yet.
>> 
>> Recommendation for review: the old file was huge. Finding the new code in the diff can be hard. I would start by only reading the new file.
>> 
>> Ah. And about runtime of the test. On my machine I get this (in ms):
>> 
>> Generate: 27
>> Compile:  5845
>> Run:      23435
>> 
>> Test generation is negligible. 6sec on compilation, 20+sec on execution. I think that is an ok balance, at least we can say that generation and compilation only take about 1/6 of total time - an acceptable overhead I would say.
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   add @compile for IR Framework

That's a very nice application and proof of concept for the Compile Framework! It's quite concise and easier to understand what's being tested. Only some minor, mostly style comments, otherwise, looks good to me, too!

test/hotspot/jtreg/compiler/loopopts/superword/TestDependencyOffsets.java line 29:

> 27:  * @summary Test SuperWord: vector size, offsets, dependencies, alignment.
> 28:  * @library /test/lib /
> 29:  * @compile ../../lib/ir_framework/TestFramework.java

Since this is quite a significant test change, do you wanna the this JBS number as well?

test/hotspot/jtreg/compiler/loopopts/superword/TestDependencyOffsets.java line 477:

> 475:                Arrays.stream(types).map(type -> type.generateInit()).collect(Collectors.joining("\n")),
> 476:                Arrays.stream(types).map(type -> type.generateVerify()).collect(Collectors.joining("\n")),
> 477:                getTests().stream().map(test -> test.generate()).collect(Collectors.joining("\n")));

Since you don't need the lambda parameter, you can directly use a method reference:
Suggestion:

               Arrays.stream(types).map(Type::generateInit).collect(Collectors.joining("\n")),
               Arrays.stream(types).map(Type::generateVerify).collect(Collectors.joining("\n")),
               getTests().stream().map(TestDefinition::generate).collect(Collectors.joining("\n")));

test/hotspot/jtreg/compiler/loopopts/superword/TestDependencyOffsets.java line 480:

> 478:     }
> 479: 
> 480:     public static void main(String args[]) {

While at it:
Suggestion:

    public static void main(String[] args) {

test/hotspot/jtreg/compiler/loopopts/superword/TestDependencyOffsets.java line 575:

> 573:     }
> 574: 
> 575:     static Type[] types = new Type[] {

Can be made final, maybe also put in capital letters.
Suggestion:

    static final Type[] TYPES = new Type[] {

test/hotspot/jtreg/compiler/loopopts/superword/TestDependencyOffsets.java line 610:

> 608:         Set<Integer> set = Arrays.stream(always).boxed().collect(Collectors.toSet());
> 609: 
> 610:         // Sample some random values on an exponental scale

Suggestion:

        // Sample some random values on an exponential scale

test/hotspot/jtreg/compiler/loopopts/superword/TestDependencyOffsets.java line 619:

> 617: 
> 618:         List<Integer> offsets = new ArrayList<Integer>(set);
> 619:         return offsets;

Can be simplified to:
Suggestion:

        return new ArrayList<>(set);

test/hotspot/jtreg/compiler/loopopts/superword/TestDependencyOffsets.java line 630:

> 628:         String generate() {
> 629:             int start = offset >= 0 ? 0 : -offset;
> 630:             String end = offset >=0 ? "SIZE - " + offset : "SIZE";

Suggestion:

            String end = offset >= 0 ? "SIZE - " + offset : "SIZE";

test/hotspot/jtreg/compiler/loopopts/superword/TestDependencyOffsets.java line 630:

> 628:         String generate() {
> 629:             int start = offset >= 0 ? 0 : -offset;
> 630:             String end = offset >=0 ? "SIZE - " + offset : "SIZE";

Suggestion:

            String end = offset >= 0 ? "SIZE - " + offset : "SIZE";

test/hotspot/jtreg/compiler/loopopts/superword/TestDependencyOffsets.java line 635:

> 633:             String secondArgument;
> 634:             String loadFrom;
> 635:             switch(RANDOM.nextInt(3)) {

Suggestion:

            switch (RANDOM.nextInt(3)) {

test/hotspot/jtreg/compiler/loopopts/superword/TestDependencyOffsets.java line 736:

> 734:             IRRule r1 = new IRRule(type, type.irNode);
> 735:             r1.addApplyIf("\"AlignVector\", \"false\"");
> 736:             r1.addApplyIf("\"MaxVectorSize\", \">=" + minVectorWidth + "\"");

Maybe at some point, the Compile Framework could offer such an IR rule builder class. But might be something for a future RFE.

test/hotspot/jtreg/compiler/loopopts/superword/TestDependencyOffsets.java line 790:

> 788: 
> 789:     static List<TestDefinition> getTests() {
> 790:         List<TestDefinition> tests = new ArrayList<TestDefinition>();

Suggestion:

        List<TestDefinition> tests = new ArrayList<>();

test/hotspot/jtreg/compiler/loopopts/superword/TestDependencyOffsets.java line 847:

> 845:                     builder.append(applyIf.size() > 1 ? "And" : "");
> 846:                     builder.append(" = {");
> 847:                     builder.append(applyIf.stream().collect(Collectors.joining(", ")));

Can be replaced with:
Suggestion:

                    builder.append(String.join(", ", applyIf));

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21541#pullrequestreview-2387165420
PR Review Comment: https://git.openjdk.org/jdk/pull/21541#discussion_r1811866151
PR Review Comment: https://git.openjdk.org/jdk/pull/21541#discussion_r1811870865
PR Review Comment: https://git.openjdk.org/jdk/pull/21541#discussion_r1811873196
PR Review Comment: https://git.openjdk.org/jdk/pull/21541#discussion_r1811883958
PR Review Comment: https://git.openjdk.org/jdk/pull/21541#discussion_r1811885149
PR Review Comment: https://git.openjdk.org/jdk/pull/21541#discussion_r1811888095
PR Review Comment: https://git.openjdk.org/jdk/pull/21541#discussion_r1811890440
PR Review Comment: https://git.openjdk.org/jdk/pull/21541#discussion_r1811890666
PR Review Comment: https://git.openjdk.org/jdk/pull/21541#discussion_r1811890836
PR Review Comment: https://git.openjdk.org/jdk/pull/21541#discussion_r1811919124
PR Review Comment: https://git.openjdk.org/jdk/pull/21541#discussion_r1811924861
PR Review Comment: https://git.openjdk.org/jdk/pull/21541#discussion_r1811921197