8271203: C2: assert(iff->Opcode() == Op_If || iff->Opcode() == Op_CountedLoopEnd || iff->Opcode() == Op_RangeCheck) failed: Check this code when new subtype is added #4920

y1yang0 · 2021-07-28T09:54:29Z

Hi, I'm trying to fix JDK-8271203. It's reasonable to unswitch LongCountedLoop, so relax it.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8271203: C2: assert(iff->Opcode() == Op_If || iff->Opcode() == Op_CountedLoopEnd || iff->Opcode() == Op_RangeCheck) failed: Check this code when new subtype is added

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer) ⚠️ Review applies to c2d47b2
Roland Westrelin (@rwestrel - Reviewer) ⚠️ Review applies to c2d47b2
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4920/head:pull/4920
$ git checkout pull/4920

Update a local copy of the PR:
$ git checkout pull/4920
$ git pull https://git.openjdk.java.net/jdk pull/4920/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4920

View PR using the GUI difftool:
$ git pr show -t 4920

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4920.diff

bridgekeeper · 2021-07-28T09:54:56Z

👋 Welcome back yyang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-07-28T09:58:38Z

@kelthuzadx The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-07-28T10:03:46Z

Webrevs

chhagedorn

I don't think that a LongCountedLoop is unswitched here looking at the stack trace but it's rather chosen as unswitch If. I had a closer look at it. It seems that the loop

for (long l1 = i; l1 < 2; ++l1) {
     iArrFld[0] += 5;
}

is peeled once and then the original loop is found to be dead (because it is known that the type of the iv i of the outer-most loop is >= 1). From this peeled iteration we only have 403 LongCountedLoopEnd left which now just acts as a normal If node. It looks fine to then simply use this as an unswitch if for unswitching 367 CountedLoop. So, relaxing the assert to allow a LongCountedLoopEnd node in dominated_by looks good IMO.

openjdk · 2021-07-29T09:13:55Z

@kelthuzadx This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8271203: C2: assert(iff->Opcode() == Op_If || iff->Opcode() == Op_CountedLoopEnd || iff->Opcode() == Op_RangeCheck) failed: Check this code when new subtype is added

Reviewed-by: chagedorn, roland, thartmann

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 300 new commits pushed to the master branch:

d06d0b9: 8272112: Arena code simplifications
0c4be76: 8058704: Nimbus does not honor JTextPane background color
020aec5: 8271366: [REDO] JDK-8266054 VectorAPI rotate operation optimization
4d4ba5c: 8272116: Update PerfDisableSharedMem with FLAG_SET_ERGO in PerfMemory::create_memory_region
09ab86b: 8269909: getStack method in hprof.parser.Reader should use try-with-resource
e4766ee: 8272391: Undeleted debug information
428d516: 8140241: (fc) Data transfer from FileChannel to itself causes hang in case of overlap
93cab7d: 8272315: Improve assert_different_registers
9980b41: 8272107: Removal of Unsafe::defineAnonymousClass left a dangling C++ class
464e874: 8048190: NoClassDefFoundError omits original ExceptionInInitializerError
... and 290 more: https://git.openjdk.java.net/jdk/compare/1eeb1791abc438a65fa3491d1cd14ad69ef1ff98...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

rwestrel

Looks good to me.

TobiHartmann · 2021-07-30T10:01:25Z

test/hotspot/jtreg/compiler/c2/UnswitchingLongCountedLoop.java

+ * @bug 8271203
+ * @summary C2: assert(iff->Opcode() == Op_If || iff->Opcode() == Op_CountedLoopEnd || iff->Opcode() == Op_RangeCheck) failed: Check this code when new subtype is added
+ * @library /test/lib
+ * @run main/othervm -Xbatch -XX:-TieredCompilation -XX:CompileOnly=UnswitchingLongCountedLoop compiler.c2.UnswitchingLongCountedLoop


Isn't that CompileOnly statement missing the package name or a wildcard?

And @library /test/lib is not needed.

Hi Tobias, I've removed @library /test/lib, also according to @chhagedorn 's analysis, I think LongCountedLoopAsUnswitchIff should be a better test name.

But the CompileOnly statement is still missing the package name, right? No compilations will happen.

Oops...Added

y1yang0 · 2021-08-13T10:42:08Z

I don't think that a LongCountedLoop is unswitched here looking at the stack trace but it's rather chosen as unswitch If. I had a closer look at it. It seems that the loop
for (long l1 = i; l1 < 2; ++l1) {
     iArrFld[0] += 5;
}
is peeled once and then the original loop is found to be dead (because it is known that the type of the iv i of the outer-most loop is >= 1). From this peeled iteration we only have 403 LongCountedLoopEnd left which now just acts as a normal If node. It looks fine to then simply use this as an unswitch if for unswitching 367 CountedLoop. So, relaxing the assert to allow a LongCountedLoopEnd node in dominated_by looks good IMO.

Hi Christian, I think you are right. As far as I see, after some loop optimizations, loop at line-17 and loop at line-19 are no longer real loops after PhaseIdealLoop2:

I'm not sure why there are two same loops(B9 and B19-B22) .. but anyway, 403#LongCountedLoopEnd is located in B21, it's not a loop anymore since no backedge found and C2 treats it as a unswitch_iff.

y1yang0 · 2021-08-13T10:58:17Z

Thanks @chhagedorn @rwestrel and @TobiHartmann for reviews.

TobiHartmann

Looks good to me but please verify that the test reproduces the issue without your fix.

y1yang0 · 2021-08-13T11:59:03Z

Looks good to me but please verify that the test reproduces the issue without your fix.

Yes, I will test if it would crash without this fix before integrating.

chhagedorn · 2021-08-13T12:25:25Z

I don't think that a LongCountedLoop is unswitched here looking at the stack trace but it's rather chosen as unswitch If. I had a closer look at it. It seems that the loop
for (long l1 = i; l1 < 2; ++l1) {
     iArrFld[0] += 5;
}
is peeled once and then the original loop is found to be dead (because it is known that the type of the iv i of the outer-most loop is >= 1). From this peeled iteration we only have 403 LongCountedLoopEnd left which now just acts as a normal If node. It looks fine to then simply use this as an unswitch if for unswitching 367 CountedLoop. So, relaxing the assert to allow a LongCountedLoopEnd node in dominated_by looks good IMO.
Hi Christian, I think you are right. As far as I see, after some loop optimizations, loop at line-17 and loop at line-19 are no longer real loops after PhaseIdealLoop2:

I'm not sure why there are two same loops(B9 and B19-B22) .. but anyway, 403#LongCountedLoopEnd is located in B21, it's not a loop anymore since no backedge found and C2 treats it as a unswitch_iff.

Not sure which lines you refer to in your image. Lines in the JTreg test (they do not match)? And at what point is this image taken? Can you also elaborate more on what you mean by two same loops?

y1yang0 · 2021-08-13T12:45:10Z

JTREG TEST:


public class Reduced {
    static int iArrFld[] = new int[400];

    public static void main(String[] strArr) {
        for (int i = 0; i < 10; i++) {
            test();
        }
    }

    static void test() {
        int i = 56, i2 = 22257;

        do {
            do {
                int i24 = 1;
                while (++i24 < 2) {
                }
                for (long l1 = i; l1 < 2; ++l1) {
                    iArrFld[0] += 5;
                }
            } while ((i2 -= 2) > 0);  //line 22
            for (long l3 = 8; l3 < 194; ++l3) { // line 23
            }
        } while (--i > 0);  //line25
    }
}

Well, the above CFG is corresponding to the last step(before beautify loops, loop.xml.log ) before crash. I haven't learned about loopopts in C2 yet, it looks like B9 and B19-22 are refer to the same loop(do{...} while ((i2 -= 2) > 0); ), the above CFG looks like:

 static void test() {
        int i = 56, i2 = 22257;

        do {
             if (xx) {
               do{
                 ....  // for (long l1 = i; l1 < 2; ++l1)  and while (++i24 < 2)  does not appear here
                } while ((i2 -= 2) > 0); 
             } else {
               do{ 
                  .... //  for (long l1 = i; l1 < 2; ++l1)  and while (++i24 < 2)  does not appear here
               } while ((i2 -= 2) > 0); 
             }
            for (long l3 = 8; l3 < 194; ++l3) { // line 23
            }
        } while (--i > 0);  //line25
    }

...maybe that's not true.

chhagedorn · 2021-08-13T13:04:52Z

Well, the above CFG is corresponding to the last step(before beautify loops, loop.xml.log ) before crash. I haven't learned about loopopts in C2 yet, it looks like B9 and B19-22 are refer to the same loop(do{...} while ((i2 -= 2) > 0); ), the above CFG looks like:

They probably appear to be very similar on first sight but are not. You can check the -XX:+TraceLoopOpts output to see what's going on with your loops. Maybe the loop was already unswitched once and now you have 2 very similar looking loops that correspond to the loop do{...} while ((i2 -= 2) > 0); ) in the Java code. In general, there is not always a 1:1 relation between a loop in Java code and LoopNode(s) in the IR. For example, you could also be dealing with pre/main/post loops for a single Java loop which look very similar.

y1yang0 · 2021-08-13T13:16:16Z

Well, the above CFG is corresponding to the last step(before beautify loops, loop.xml.log ) before crash. I haven't learned about loopopts in C2 yet, it looks like B9 and B19-22 are refer to the same loop(do{...} while ((i2 -= 2) > 0); ), the above CFG looks like:

They probably appear to be very similar on first sight but are not. You can check the -XX:+TraceLoopOpts output to see what's going on with your loops. Maybe the loop was already unswitched once and now you have 2 very similar looking loops that correspond to the loop do{...} while ((i2 -= 2) > 0); ) in the Java code. In general, there is not always a 1:1 relation between a loop in Java code and LoopNode(s) in the IR. For example, you could also be dealing with pre/main/post loops for a single Java loop which look very similar.

Make sense. TraceLoopOpts may tell me everything about what happens. I’m curious about loop optimizations, this is a good start point to explore them:P

y1yang0 · 2021-08-16T02:29:31Z

/integrate

openjdk · 2021-08-16T02:31:32Z

Going to push as commit 3f38a50.
Since your change was applied there have been 307 commits pushed to the master branch:

6a5241c: 8272491: Problem list javax/swing/JFrame/NSTexturedJFrame/NSTexturedJFrame.java on macos
17b9350: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL
87d2761: 8271883: Math CopySign optimization for x86
6b8b160: 8272396: mismatching debug output streams
0af645a: 8205138: Remove Applet references from Font2DTest
bd7f9b4: 8272459: ProblemList compiler/codecache/TestStressCodeBuffers.java on aarch64
717792c: 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path
d06d0b9: 8272112: Arena code simplifications
0c4be76: 8058704: Nimbus does not honor JTextPane background color
020aec5: 8271366: [REDO] JDK-8266054 VectorAPI rotate operation optimization
... and 297 more: https://git.openjdk.java.net/jdk/compare/1eeb1791abc438a65fa3491d1cd14ad69ef1ff98...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-08-16T02:31:51Z

@kelthuzadx Pushed as commit 3f38a50.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

try

c2d47b2

openjdk bot added the rfr Pull request is ready for review label Jul 28, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jul 28, 2021

chhagedorn approved these changes Jul 29, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 29, 2021

rwestrel approved these changes Jul 30, 2021

View reviewed changes

TobiHartmann suggested changes Jul 30, 2021

View reviewed changes

y1yang0 added 2 commits August 13, 2021 18:59

rename test

4775994

package

9769de9

TobiHartmann approved these changes Aug 13, 2021

View reviewed changes

openjdk bot closed this Aug 16, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 16, 2021

y1yang0 deleted the JDK-8271203 branch August 16, 2021 02:32

8271203: C2: assert(iff->Opcode() == Op_If || iff->Opcode() == Op_CountedLoopEnd || iff->Opcode() == Op_RangeCheck) failed: Check this code when new subtype is added #4920

8271203: C2: assert(iff->Opcode() == Op_If || iff->Opcode() == Op_CountedLoopEnd || iff->Opcode() == Op_RangeCheck) failed: Check this code when new subtype is added #4920

y1yang0 commented Jul 28, 2021 •

edited by openjdk bot

bridgekeeper bot commented Jul 28, 2021

openjdk bot commented Jul 28, 2021

mlbridge bot commented Jul 28, 2021 •

edited

chhagedorn left a comment •

edited

openjdk bot commented Jul 29, 2021 •

edited

rwestrel left a comment

TobiHartmann Jul 30, 2021 •

edited

TobiHartmann Jul 30, 2021

y1yang0 Aug 13, 2021

TobiHartmann Aug 13, 2021

y1yang0 Aug 13, 2021

y1yang0 commented Aug 13, 2021

y1yang0 commented Aug 13, 2021

TobiHartmann left a comment

y1yang0 commented Aug 13, 2021

chhagedorn commented Aug 13, 2021

y1yang0 commented Aug 13, 2021 •

edited

chhagedorn commented Aug 13, 2021

y1yang0 commented Aug 13, 2021

y1yang0 commented Aug 16, 2021

openjdk bot commented Aug 16, 2021

openjdk bot commented Aug 16, 2021

8271203: C2: assert(iff->Opcode() == Op_If || iff->Opcode() == Op_CountedLoopEnd || iff->Opcode() == Op_RangeCheck) failed: Check this code when new subtype is added #4920

8271203: C2: assert(iff->Opcode() == Op_If || iff->Opcode() == Op_CountedLoopEnd || iff->Opcode() == Op_RangeCheck) failed: Check this code when new subtype is added #4920

Conversation

y1yang0 commented Jul 28, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 28, 2021

openjdk bot commented Jul 28, 2021

mlbridge bot commented Jul 28, 2021 • edited

Webrevs

chhagedorn left a comment • edited

Choose a reason for hiding this comment

openjdk bot commented Jul 29, 2021 • edited

rwestrel left a comment

Choose a reason for hiding this comment

TobiHartmann Jul 30, 2021 • edited

Choose a reason for hiding this comment

TobiHartmann Jul 30, 2021

Choose a reason for hiding this comment

y1yang0 Aug 13, 2021

Choose a reason for hiding this comment

TobiHartmann Aug 13, 2021

Choose a reason for hiding this comment

y1yang0 Aug 13, 2021

Choose a reason for hiding this comment

y1yang0 commented Aug 13, 2021

y1yang0 commented Aug 13, 2021

TobiHartmann left a comment

Choose a reason for hiding this comment

y1yang0 commented Aug 13, 2021

chhagedorn commented Aug 13, 2021

y1yang0 commented Aug 13, 2021 • edited

chhagedorn commented Aug 13, 2021

y1yang0 commented Aug 13, 2021

y1yang0 commented Aug 16, 2021

openjdk bot commented Aug 16, 2021

openjdk bot commented Aug 16, 2021

y1yang0 commented Jul 28, 2021 •

edited by openjdk bot

mlbridge bot commented Jul 28, 2021 •

edited

chhagedorn left a comment •

edited

openjdk bot commented Jul 29, 2021 •

edited

TobiHartmann Jul 30, 2021 •

edited

y1yang0 commented Aug 13, 2021 •

edited