Skip to content
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

8266854: LibraryCallKit::inline_preconditions_checkIndex modifies control flow even if the intrinsic bailed out #3958

Closed
wants to merge 3 commits into from

Conversation

@sviswa7
Copy link

@sviswa7 sviswa7 commented May 10, 2021

LibraryCallKit::inline_preconditions_checkIndex can result in the following assert sometimes:
"# assert(ctrl == kit.control()) failed: Control flow was added although the intrinsic bailed out"

Consider the following code snippet:
...
set_control(_gvn.transform(new IfTrueNode(rc)));
{
PreserveJVMState pjvms(this);
set_control(_gvn.transform(new IfFalseNode(rc)));
uncommon_trap(Deoptimization::Reason_range_check,
Deoptimization::Action_make_not_entrant);
}
..
Here the control is being modified by set_control even though a bailout is possible afterwards.
Moving the set_control later in the intrinsic fixes this.

This is a small fix. Please review.

Best Regards,
Sandhya


Progress

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

Issue

  • JDK-8266854: LibraryCallKit::inline_preconditions_checkIndex modifies control flow even if the intrinsic bailed out

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3958

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 10, 2021

👋 Welcome back sviswanathan! 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.

Loading

@sviswa7
Copy link
Author

@sviswa7 sviswa7 commented May 10, 2021

/label hotspot-compiler

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 10, 2021

@sviswa7
The hotspot-compiler label was successfully added.

Loading

@sviswa7 sviswa7 marked this pull request as ready for review May 10, 2021
@openjdk openjdk bot added the rfr label May 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 10, 2021

Webrevs

Loading

@@ -1046,10 +1046,11 @@ bool LibraryCallKit::inline_preconditions_checkIndex(BasicType bt) {
if (!rc_bool->is_Con()) {
record_for_igvn(rc);
}
set_control(_gvn.transform(new IfTrueNode(rc)));
Copy link
Contributor

@vnkozlov vnkozlov May 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you can move stopped() check here (after set_control()) and avoid generation of useless false path.

Loading

Copy link
Author

@sviswa7 sviswa7 May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vnkozlov for the review. It is this set_control() that is resulting in an assert that control is changed but the intrinsic bailed out. Moving the stopped check right before set_control() would fix the issue. I will update the patch accordingly.

Loading

Copy link
Contributor

@vnkozlov vnkozlov May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange. Actually BuildCutout also change control and we have stopped() check after it at line #1036.
After that check there is no control change so checking stopped() again here looks strange if it helps.
May be this code missing some compile time checks for constants (like TOP) which collapse this graph so we can bailout it without generating code.
Note, the assert is correct - we should not change graph too mach. It should be dead if returns false and it will do normal compilation of the bytecode.
Need more information about this case.

Loading

{
PreserveJVMState pjvms(this);
set_control(_gvn.transform(new IfFalseNode(rc)));
uncommon_trap(Deoptimization::Reason_range_check,
Deoptimization::Action_make_not_entrant);
}

if (stopped()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving the check around doesn't make much sense to me.

stopped() == false signals that the current control is effectively dead. It could happen when the range check (RangeCheckNode ) always fails and execution unconditionally hits the uncommon trap. (I haven't double-checked myself whether it happens in practice or not.)

By bailing out from the intrinsic (return false;), the next thing C2 will attempt is to inline `Preconditions::checkIndex(). There were no attempts to clean up the graph built up to this point (with the uncommon trap).

Instead, the fix can just be if (stopped()) { return true; }. The graph constructed so far is valid.

Loading

Copy link
Author

@sviswa7 sviswa7 May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'if (stopped()) { return true; }' (i.e. only changing the return false to return true at original line 1058) also fixes the issue.
It is something to do with range check fails.

I discovered this issue while working on vector api. I had a call to Objects.checkIndex(origin, length()) and came across the 'assert(ctrl == kit.control()) failed'. As part of code review we found that the call should have been Objects.checkIndex(origin, length()+1) so it looks like something to do with range check fails. I am unable to create a stand alone test case.

Let me know if we should go ahead with the 'if (stopped()) { return true; }' fix.

Loading

Copy link
Contributor

@vnkozlov vnkozlov May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, returning true could be correct because of constant folding we end up in uncommon trap which is valid for provided values (constants). It will be the same if code is compiled normally.
But we also need to move the other stopped() check from line 1036 to 1028 after BuildCutout which may also can go to uncommon trap (when length is know negative during compilation). And it also should return true.

Loading

@sviswa7
Copy link
Author

@sviswa7 sviswa7 commented May 12, 2021

@vnkozlov @iwanowww I have implemented your review comments. Please let me know if the changes look good now.

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Looks good but it would be nice to have a test which verifies such edge cases for this intrinsic.

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 9, 2021

@sviswa7 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Jun 21, 2021

@sviswa7, do you plan to complete this PR and have a new test for JDK 17?

Loading

@sviswa7
Copy link
Author

@sviswa7 sviswa7 commented Jun 21, 2021

@vnkozlov I wasn't able to create a test.

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Jun 21, 2021

Can you file a separate RFE to write a test and proceed with this PR?

Loading

@sviswa7
Copy link
Author

@sviswa7 sviswa7 commented Jun 21, 2021

Yes, I will withdraw this and create a PR versus JDK 17 for the fix.
Also will file a RFE for the test separately.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants