New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8283187: C2: loop candidate for superword not always unrolled fully if superword fails #7822
Conversation
|
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! The fix looks good to me.
test/hotspot/jtreg/compiler/c2/irTests/TestSuperwordFailsUnrolling.java
Outdated
Show resolved
Hide resolved
@rwestrel This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
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 24 new commits pushed to the
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.
|
…ling.java Co-authored-by: Tobias Hartmann <tobias.hartmann@oracle.com>
thanks for the review! I applied your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good!
src/hotspot/share/opto/superword.cpp
Outdated
if (!cl->is_valid_counted_loop(T_INT)) return false; // skip malformed counted loop | ||
|
||
bool post_loop_allowed = (PostLoopMultiversioning && Matcher::has_predicated_vectors() && cl->is_post_loop()); | ||
if (post_loop_allowed) { | ||
if (cl->is_reduction_loop()) return; // no predication mapping | ||
if (cl->is_reduction_loop()) return false; // no predication mapping | ||
Node *limit = cl->limit(); | ||
if (limit->is_Con()) return; // non constant limits only | ||
if (limit->is_Con()) return false; // non constant limits only | ||
// Now check the limit for expressions we do not handle | ||
if (limit->is_Add()) { | ||
Node *in2 = limit->in(2); | ||
if (in2->is_Con()) { | ||
int val = in2->get_int(); | ||
// should not try to program these cases | ||
if (val < 0) return; | ||
if (val < 0) return false; | ||
} | ||
} | ||
} | ||
|
||
// skip any loop that has not been assigned max unroll by analysis | ||
if (do_optimization) { | ||
if (SuperWordLoopUnrollAnalysis && cl->slp_max_unroll() == 0) return; | ||
if (SuperWordLoopUnrollAnalysis && cl->slp_max_unroll() == 0) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While at it, you could also add braces for the one liner ifs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be done in the new commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I executed some testing and compiler/c2/irTests/TestSuperwordFailsUnrolling.java
fails on Windows only with:
java.lang.Error: failed to install whitebox classes
at compiler.lib.ir_framework.TestFramework.installWhiteBox(TestFramework.java:529)
at compiler.lib.ir_framework.TestFramework.start(TestFramework.java:318)
at compiler.lib.ir_framework.TestFramework.run(TestFramework.java:210)
at compiler.lib.ir_framework.TestFramework.run(TestFramework.java:199)
at compiler.c2.irTests.TestSuperwordFailsUnrolling.main(TestSuperwordFailsUnrolling.java:48)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
at java.base/java.lang.reflect.Method.invoke(Method.java:577)
at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:828)
Caused by: java.nio.file.FileSystemException: sun\\hotspot\\WhiteBox.class: The process cannot access the file because it is being used by another process
at java.base/sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:92)
at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:103)
at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:108)
at java.base/sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProvider.java:275)
at java.base/sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:110)
at java.base/java.nio.file.Files.deleteIfExists(Files.java:1191)
at java.base/java.nio.file.Files.copy(Files.java:3149)
at jdk.test.lib.helpers.ClassFileInstaller.writeToDisk(ClassFileInstaller.java:296)
at jdk.test.lib.helpers.ClassFileInstaller.writeClassToDisk(ClassFileInstaller.java:255)
at jdk.test.lib.helpers.ClassFileInstaller.writeClassToDisk(ClassFileInstaller.java:237)
at jdk.test.lib.helpers.ClassFileInstaller.writeClassToDisk(ClassFileInstaller.java:234)
at jdk.test.lib.helpers.ClassFileInstaller.main(ClassFileInstaller.java:97)
at compiler.lib.ir_framework.TestFramework.installWhiteBox(TestFramework.java:527)
... 8 more
Weird but reproducible.
Let me try to reproduce with |
This fixes the issue:
@chhagedorn Maybe the IR verification framework could detect this automatically? |
Yes, it would be good to extend the framework to handle this. I added this |
New commit has this change |
Thanks for the review @chhagedorn |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the updates!
src/hotspot/share/opto/superword.cpp
Outdated
if (!cl->is_valid_counted_loop(T_INT)) return false; // skip malformed counted loop | ||
|
||
bool post_loop_allowed = (PostLoopMultiversioning && Matcher::has_predicated_vectors() && cl->is_post_loop()); | ||
if (post_loop_allowed) { | ||
if (cl->is_reduction_loop()) return; // no predication mapping | ||
if (cl->is_reduction_loop()) return false; // no predication mapping | ||
Node *limit = cl->limit(); | ||
if (limit->is_Con()) return; // non constant limits only | ||
if (limit->is_Con()) return false; // non constant limits only | ||
// Now check the limit for expressions we do not handle | ||
if (limit->is_Add()) { | ||
Node *in2 = limit->in(2); | ||
if (in2->is_Con()) { | ||
int val = in2->get_int(); | ||
// should not try to program these cases | ||
if (val < 0) return; | ||
if (val < 0) return false; | ||
} | ||
} | ||
} | ||
|
||
// skip any loop that has not been assigned max unroll by analysis | ||
if (do_optimization) { | ||
if (SuperWordLoopUnrollAnalysis && cl->slp_max_unroll() == 0) return; | ||
if (SuperWordLoopUnrollAnalysis && cl->slp_max_unroll() == 0) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While at it, you could also add braces for the one liner ifs.
/integrate |
Going to push as commit 14c20bc.
Your commit was automatically rebased without conflicts. |
IdealLoopTree::policy_unroll_slp_analysis() restricts unrolling of
loops that are found to be candidate for superword but if superword
then fails, unrolling doesn't always resume. SuperWord::output() has
logic for that in one of the failure paths but some other paths are
not covered (for instance in the case of the test case). I propose
that superword explictly returns whether it succeeded or not and that
in case of failure, for a main loop, unrolling be attempted again.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7822/head:pull/7822
$ git checkout pull/7822
Update a local copy of the PR:
$ git checkout pull/7822
$ git pull https://git.openjdk.java.net/jdk pull/7822/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 7822
View PR using the GUI difftool:
$ git pr show -t 7822
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7822.diff