-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8302459: Missing late inline cleanup causes compiler/vectorapi/VectorLogicalOpIdentityTest.java IR failure #21682
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
Changes from all commits
2f10a92
3ba3823
80901d4
7ec8069
4131271
f29f7a8
a78798c
bffdbbd
7481cc1
15cbe15
0b7dff8
f547d89
49c8a51
11c63b5
63b749c
bd488a9
f759c80
7064598
9f039fd
af130f9
ce27ae3
00917c5
a36199e
127a21b
99eaf0a
079d475
15246bc
7a4ffc1
b05308f
4f0a890
13a2d43
1c74725
a0fb800
65e7de0
7de8d12
f36c5b7
1b22923
ba3547d
2521e85
e71e72f
d688fcf
887dd96
9406c6e
bbaf985
cfa5252
f84cafb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2029,6 +2029,7 @@ bool Compile::inline_incrementally_one() { | |
| for (int i = 0; i < _late_inlines.length(); i++) { | ||
| _late_inlines_pos = i+1; | ||
| CallGenerator* cg = _late_inlines.at(i); | ||
| bool is_scheduled_for_igvn_before = C->igvn_worklist()->member(cg->call_node()); | ||
| bool does_dispatch = cg->is_virtual_late_inline() || cg->is_mh_late_inline(); | ||
| if (inlining_incrementally() || does_dispatch) { // a call can be either inlined or strength-reduced to a direct call | ||
| cg->do_late_inline(); | ||
|
|
@@ -2039,6 +2040,16 @@ bool Compile::inline_incrementally_one() { | |
| _late_inlines_pos = i+1; // restore the position in case new elements were inserted | ||
| print_method(PHASE_INCREMENTAL_INLINE_STEP, 3, cg->call_node()); | ||
| break; // process one call site at a time | ||
| } else { | ||
| bool is_scheduled_for_igvn_after = C->igvn_worklist()->member(cg->call_node()); | ||
| if (!is_scheduled_for_igvn_before && is_scheduled_for_igvn_after) { | ||
| // Avoid potential infinite loop if node already in the IGVN list | ||
| assert(false, "scheduled for IGVN during inlining attempt"); | ||
| } else { | ||
| // Ensure call node has not disappeared from IGVN worklist during a failed inlining attempt | ||
| assert(!is_scheduled_for_igvn_before || is_scheduled_for_igvn_after, "call node removed from IGVN list during inlining pass"); | ||
| cg->call_node()->set_generator(cg); | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this a bit hard to read. Wouldn't it be semantically equivalent to this? We wouldn't have separate asserts for the two cases, but I think that's fine since one can easily figure it out from the boolean values.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The difference is whether a call can be scheduled for a repeated inlining attempt in the future.
The first check guards against a situation when the call node is already on IGVN list (so, it will be automatically rescheduled for inlining during the next IGVN pass causing an infinite loop in incremental inlining). The second assert catches a suspicious situation when the call node disappears from IGVN worklist during failed inlining attempt. IMO it should not happens, hence the assert. But it is benign to allow repeated inlining in such case.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, thanks for the background. If we keep both asserts, some comments explaining this would be helpful.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good to me, thanks! |
||
| } | ||
| } else { | ||
| // Ignore late inline direct calls when inlining is not allowed. | ||
|
|
||
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.
FTR, could you share how the PrintInlining output looks now when this code is triggered?
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.
It looks like this:
It seems a bit redundant: the first
failed to inline: failed to inline (intrinsic)doesn't seem to be needed.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.
Actually it is a bit verbose but I would probably leave it like this: the first (
failed to inline: failed to inline (intrinsic)) is for the failure and the second (failed to inline: static call node changed: trying again (intrinsic)) is for the retry.