8385855: C2: IfNode::filtered_int_type should only allow CmpI#31395
8385855: C2: IfNode::filtered_int_type should only allow CmpI#31395eme64 wants to merge 45 commits into
Conversation
|
👋 Welcome back epeter! A progress list of the required criteria for merging this PR into |
|
@eme64 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: 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 12 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. ➡️ To integrate this PR with the above commit message to the |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
Webrevs
|
| // Val is always the lhs of the comparision: val <test> cmp2 | ||
| if (cmp->in(1) == val) { | ||
| assert(cmp->Opcode() == Op_CmpI, "signed comparison required"); | ||
| if (cmp->Opcode() != Op_CmpI) { |
There was a problem hiding this comment.
I will be a little more logical to have this just below const CmpNode* cmp = bol->in(1)->as_Cmp() instead of below the if.
There was a problem hiding this comment.
Alternatively,
if (cmp->Opcode() == Op_CmpI && cmp->in(1) == val)since not entering all the ifs there, is returning nullptr, and not doing anything.
There was a problem hiding this comment.
Good idea, I'll go with Marc's suggestion.
There was a problem hiding this comment.
@marc-chevalier @merykitty Can you re-review, please?
|
Just found another failure with the attached fuzzer: |
|
@merykitty @marc-chevalier @vnkozlov Ok, I decided, I'm moving the fuzzer test to a separate RFE: #31501 This allows us to fix the other two bugs, before integrating the fuzzer:
(and there could be more bugs hiding here) The IR test should be sufficient to give us some basic coverage, especially to ensure the optimization still works. Here, we just restrict to I'll need your re-approval to integrate this to JDK28, and then I'll backport to JDK27. |
merykitty
left a comment
There was a problem hiding this comment.
Thanks for your explanation, I think that is reasonable.
|
@marc-chevalier @vnkozlov @merykitty Thanks for the reviews, suggestions and approvals! /integrate |
|
Going to push as commit 841e288.
Your commit was automatically rebased without conflicts. |
|
/backport jdk:jdk27 |
|
@eme64 the backport was successfully created on the branch backport-eme64-841e2882-jdk27 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk27, just click the following link: The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:
If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk: |
Don't be fooled by the many changed lines: it is mostly testing
TLDR: The new assert revealed a bug, so I upgraded it to a condition.
Added lots of testing:
CountedLoop. Because the patterns are very brittle, and the optimization does not recognize all patterns it probably was intended for, I have lots of negative examples too. But at least the cases I do now have IR rules for will not deteriorate further.Fuzzer test: moved to #31501, which covers
CountedLoopConverter::has_truncation_wrap.I originally also developed a template fuzzer for the optimization, but then during CI, discovered more bugs with it. So I decided to split the fuzzer test into a separate RFE, to avoid issues when backporting this fix to JDK27.
CountedLoopConverter::has_truncation_wrap, with subword iv loops.Bug 1: The Fuzzer found another bug in the same general code region:
JDK-8386482 C2 CountedLoopConverter::filtered_type_from_dominators: assert(_base == Int) failed: Not an Int
PR: #31481
Bug 2: Yet another bug found by the attached fuzzer:
JDKJDK-8386591 C2: wrong result because of broken truncation check in CountedLoopConverter::TruncatedIncrement::build
Details
This was filed as a regression of #28819, but I only added an assert there, to check that
IfNode::filtered_int_typedoes not operate on anything else thanCmpI. I added that assert with high but not perfect confidence, and no test failed, so I assumed the assert should be correct.But now we found a reproducer where we get to
IfNode::filtered_int_typefromCountedLoopConverter::has_truncation_wrap. So we tripped the new assert. But it turns out that there was a bug here even before I added the assert, the assert just revealed that bug. We can get wrong results, becauseIfNode::filtered_int_typeworks with signed int ranges, and so if we use it on aCmpU, the type range will be incorrect.CountedLoopConverter::has_truncation_wraplooks at loops that truncate the iv, i.e. cast it to byte/char/short. If we can prove that the iv never would have gone out of that range, we can drop the truncation, and make it a regular int-counted-loop, i.e. aCountedLoop. Loops that keep the truncation cannot beCountedLoop.Specifically, we look before the loop, to see if the init value is in the truncated range. For this, we look for conditions with
CmpI(or as before this bug alsoCmpU), and useIfNode::filtered_int_typeto constrain the type of the init value using the comparison. But if we did this for aCmpU, and the type info was wrong, we might have concluded that the values are inside the truncated range, when they could actually go outside: wrong result!I'm now even more sure that only
CmpIshould be allowed inIfNode::filtered_int_type, because it only handles signed ranges.Reflection on the Optimization
The fix is conservative, and now only allows
CmpI. There is a risk thatIfNode::filtered_int_typeallowed other shapes to optimize, where the optimization accidentally was not incorrect. But we now know that it is only provable correct forCmpI, and provably incorrect for some other cases (regression examples forCmpU).To be honest, the
CountedLoopConverter::has_truncation_wrapseems like it has deteriorated over time, and is not super general. It has some strange limitations:!=exit checks instead of<, ... they all can make otherwise similar Java loops look different to C2. We don't recognize all shapes that could be turned into a CountedLoop.TruncatedIncrement::buildonly recognizes a subset of truncation patterns, there are some original bugs and some patterns seem to have deteriorated over time.<<24 >>24, char cast lowered through& 0xffff.shift == 8is((x << 8) >> 8)is signed 24-bit truncation. But the implementation maps to BYTE. This looks like an original "bug", which does not create any real issues, just means we miss optimizations for byte. We should probably matchshift == 24instead, which would be<<24 >>24.LShiftINode::Ideal, and so they are not matched any more,& 0x7fff, and not the actual char truncation (& 0xffff). So we don't actually turn(char)truncation into CountedLoops. Actually, this is a bug, see: JDK-8386597.is_infinite_loop:if (limit_t->hi_as_long() > incr_t->hi_as_long()) {only makes sense for positive strides. I'm fairly sure this does not lead to bugs, but again: missed optimizations.My conclusion:
CountedLoopConverter::has_truncation_wrapis VERY brittle, and has deteriorated over time. I'm also not sure how important loops withbyte,charandshortiv really are. So maybe this is not so bad. Maybe we could even remove the optimization, and would not suffer any regressions. I welcome discussion here, but propose to fix the issue here anyway, as proposed. After all theassertis aJDK27regression and should get fixed quickly.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/31395/head:pull/31395$ git checkout pull/31395Update a local copy of the PR:
$ git checkout pull/31395$ git pull https://git.openjdk.org/jdk.git pull/31395/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 31395View PR using the GUI difftool:
$ git pr show -t 31395Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/31395.diff
Using Webrev
Link to Webrev Comment