Skip to content

8358751: C2: Recursive inlining check for compiled lambda forms is broken #26891

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+75 −34
Open

8358751: C2: Recursive inlining check for compiled lambda forms is broken #26891

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 18 additions & 15 deletions src/hotspot/share/opto/bytecodeInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ InlineTree::InlineTree(Compile* c,
// Keep a private copy of the caller_jvms:
_caller_jvms = new (C) JVMState(caller_jvms->method(), caller_tree->caller_jvms());
_caller_jvms->set_bci(caller_jvms->bci());
_caller_jvms->set_receiver_info(caller_jvms->receiver_info());
assert(!caller_jvms->should_reexecute(), "there should be no reexecute bytecode with inlining");
assert(_caller_jvms->same_calls_as(caller_jvms), "consistent JVMS");
}
Expand Down Expand Up @@ -437,24 +438,26 @@ bool InlineTree::try_to_inline(ciMethod* callee_method, ciMethod* caller_method,

// detect direct and indirect recursive inlining
{
// count the current method and the callee
const bool is_compiled_lambda_form = callee_method->is_compiled_lambda_form();
int inline_level = 0;
if (!is_compiled_lambda_form) {
if (method() == callee_method) {
inline_level++;
}
const bool is_method_handle_invoker = is_compiled_lambda_form && !jvms->method()->is_compiled_lambda_form();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ignoring the bug you're fixing, is that logic expected to compute the same inline_level that the current logic computes? You changed it a bit (iterate from the current frame rather than the caller, the extra test for is_method_handle_invoker and the extra test for lform_caller_recv == nullptr in the loop that I'm not sure what the answer to that question is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a compelling reason to treat immediate caller specially here. Since current logic is broken, I decided to make the unification along with the fix. I can separate it into an RFE, if you have any concerns.


ciInstance* lform_callee_recv = nullptr;
if (is_compiled_lambda_form && !is_method_handle_invoker) { // MH invokers don't have a receiver
lform_callee_recv = jvms->compute_receiver_info(callee_method);
}
// count callers of current method and callee
Node* callee_argument0 = is_compiled_lambda_form ? jvms->map()->argument(jvms, 0)->uncast() : nullptr;
for (JVMState* j = jvms->caller(); j != nullptr && j->has_method(); j = j->caller()) {

int inline_level = 0;
for (JVMState* j = jvms; j != nullptr && j->has_method(); j = j->caller()) {
if (j->method() == callee_method) {
if (is_compiled_lambda_form) {
// Since compiled lambda forms are heavily reused we allow recursive inlining. If it is truly
// a recursion (using the same "receiver") we limit inlining otherwise we can easily blow the
// compiler stack.
Node* caller_argument0 = j->map()->argument(j, 0)->uncast();
if (caller_argument0 == callee_argument0) {
// Since compiled lambda forms are heavily reused we allow recursive inlining. If it is truly
// a recursion (using the same "receiver") we limit inlining otherwise we can easily blow the
// compiler stack.
if (lform_callee_recv != nullptr) {
ciInstance* lform_caller_recv = j->receiver_info();
assert(lform_caller_recv != nullptr || j->depth() == 1 ||
!j->caller()->method()->is_compiled_lambda_form(), // MH invoker
"missing receiver info");
if (lform_caller_recv == lform_callee_recv || lform_caller_recv == nullptr) {
inline_level++;
}
} else {
Expand Down
23 changes: 20 additions & 3 deletions src/hotspot/share/opto/callnode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,8 @@ uint TailJumpNode::match_edge(uint idx) const {

//=============================================================================
JVMState::JVMState(ciMethod* method, JVMState* caller) :
_method(method) {
_method(method),
_receiver_info(nullptr) {
assert(method != nullptr, "must be valid call site");
_bci = InvocationEntryBci;
_reexecute = Reexecute_Undefined;
Expand All @@ -278,7 +279,8 @@ JVMState::JVMState(ciMethod* method, JVMState* caller) :
_sp = 0;
}
JVMState::JVMState(int stack_size) :
_method(nullptr) {
_method(nullptr),
_receiver_info(nullptr) {
_bci = InvocationEntryBci;
_reexecute = Reexecute_Undefined;
DEBUG_ONLY(_map = (SafePointNode*)-1);
Expand Down Expand Up @@ -613,6 +615,7 @@ JVMState* JVMState::clone_shallow(Compile* C) const {
n->set_endoff(_endoff);
n->set_sp(_sp);
n->set_map(_map);
n->set_receiver_info(_receiver_info);
return n;
}

Expand Down Expand Up @@ -687,6 +690,20 @@ int JVMState::interpreter_frame_size() const {
return size + Deoptimization::last_frame_adjust(0, callee_locals) * BytesPerWord;
}

// Compute receiver info for a compiled lambda form at call site.
ciInstance* JVMState::compute_receiver_info(ciMethod* callee) const {
assert(callee != nullptr && callee->is_compiled_lambda_form(), "");
if (has_method() && method()->is_compiled_lambda_form()) { // callee is not a MH invoker
Node* recv = map()->argument(this, 0);
assert(recv != nullptr, "");
const TypeOopPtr* recv_toop = recv->bottom_type()->isa_oopptr();
if (recv_toop != nullptr && recv_toop->const_oop() != nullptr) {
return recv_toop->const_oop()->as_instance();
}
}
return nullptr;
}

//=============================================================================
bool CallNode::cmp( const Node &n ) const
{ return _tf == ((CallNode&)n)._tf && _jvms == ((CallNode&)n)._jvms; }
Expand Down Expand Up @@ -1364,7 +1381,7 @@ void CallLeafNode::dump_spec(outputStream *st) const {

//=============================================================================

void SafePointNode::set_local(JVMState* jvms, uint idx, Node *c) {
void SafePointNode::set_local(const JVMState* jvms, uint idx, Node *c) {
assert(verify_jvms(jvms), "jvms must match");
int loc = jvms->locoff() + idx;
if (in(loc)->is_top() && idx > 0 && !c->is_top() ) {
Expand Down
46 changes: 30 additions & 16 deletions src/hotspot/share/opto/callnode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ class JVMState : public ResourceObj {
int _bci; // Byte Code Index of this JVM point
ReexecuteState _reexecute; // Whether this bytecode need to be re-executed
ciMethod* _method; // Method Pointer
ciInstance* _receiver_info; // Constant receiver instance for compiled lambda forms
SafePointNode* _map; // Map node associated with this scope
public:
friend class Compile;
Expand Down Expand Up @@ -259,6 +260,7 @@ class JVMState : public ResourceObj {
bool is_reexecute_undefined() const { return _reexecute==Reexecute_Undefined; }
bool has_method() const { return _method != nullptr; }
ciMethod* method() const { assert(has_method(), ""); return _method; }
ciInstance* receiver_info() const { assert(has_method(), ""); return _receiver_info; }
JVMState* caller() const { return _caller; }
SafePointNode* map() const { return _map; }
uint depth() const { return _depth; }
Expand Down Expand Up @@ -304,13 +306,15 @@ class JVMState : public ResourceObj {
// _reexecute is initialized to "undefined" for a new bci
void set_bci(int bci) {if(_bci != bci)_reexecute=Reexecute_Undefined; _bci = bci; }
void set_should_reexecute(bool reexec) {_reexecute = reexec ? Reexecute_True : Reexecute_False;}
void set_receiver_info(ciInstance* recv) { assert(has_method() || recv == nullptr, ""); _receiver_info = recv; }

// Miscellaneous utility functions
JVMState* clone_deep(Compile* C) const; // recursively clones caller chain
JVMState* clone_shallow(Compile* C) const; // retains uncloned caller
void set_map_deep(SafePointNode *map);// reset map for all callers
void adapt_position(int delta); // Adapt offsets in in-array after adding an edge.
int interpreter_frame_size() const;
ciInstance* compute_receiver_info(ciMethod* callee) const;

#ifndef PRODUCT
void print_method_with_lineno(outputStream* st, bool show_name) const;
Expand Down Expand Up @@ -374,7 +378,7 @@ class SafePointNode : public MultiNode {
}

private:
void verify_input(JVMState* jvms, uint idx) const {
void verify_input(const JVMState* jvms, uint idx) const {
assert(verify_jvms(jvms), "jvms must match");
Node* n = in(idx);
assert((!n->bottom_type()->isa_long() && !n->bottom_type()->isa_double()) ||
Expand All @@ -383,34 +387,44 @@ class SafePointNode : public MultiNode {

public:
// Functionality from old debug nodes which has changed
Node *local(JVMState* jvms, uint idx) const {
verify_input(jvms, jvms->locoff() + idx);
return in(jvms->locoff() + idx);
Node* local(const JVMState* jvms, uint idx) const {
uint loc_idx = jvms->locoff() + idx;
assert(jvms->is_loc(loc_idx), "not a local slot");
verify_input(jvms, loc_idx);
return in(loc_idx);
}
Node *stack(JVMState* jvms, uint idx) const {
verify_input(jvms, jvms->stkoff() + idx);
return in(jvms->stkoff() + idx);
Node* stack(const JVMState* jvms, uint idx) const {
uint stk_idx = jvms->stkoff() + idx;
assert(jvms->is_stk(stk_idx), "not a stack slot");
verify_input(jvms, stk_idx);
return in(stk_idx);
}
Node *argument(JVMState* jvms, uint idx) const {
verify_input(jvms, jvms->argoff() + idx);
Node* argument(const JVMState* jvms, uint idx) const {
uint arg_idx = jvms->argoff() + idx;
assert(jvms->is_stk(arg_idx), "not an argument slot");
verify_input(jvms, arg_idx);
return in(jvms->argoff() + idx);
}
Node *monitor_box(JVMState* jvms, uint idx) const {
Node* monitor_box(const JVMState* jvms, uint idx) const {
assert(verify_jvms(jvms), "jvms must match");
return in(jvms->monitor_box_offset(idx));
uint mon_box_idx = jvms->monitor_box_offset(idx);
assert(jvms->is_monitor_box(mon_box_idx), "not a monitor box offset");
return in(mon_box_idx);
}
Node *monitor_obj(JVMState* jvms, uint idx) const {
Node* monitor_obj(const JVMState* jvms, uint idx) const {
assert(verify_jvms(jvms), "jvms must match");
return in(jvms->monitor_obj_offset(idx));
uint mon_obj_idx = jvms->monitor_obj_offset(idx);
assert(jvms->is_mon(mon_obj_idx) && !jvms->is_monitor_box(mon_obj_idx), "not a monitor obj offset");
return in(mon_obj_idx);
}

void set_local(JVMState* jvms, uint idx, Node *c);
void set_local(const JVMState* jvms, uint idx, Node *c);

void set_stack(JVMState* jvms, uint idx, Node *c) {
void set_stack(const JVMState* jvms, uint idx, Node *c) {
assert(verify_jvms(jvms), "jvms must match");
set_req(jvms->stkoff() + idx, c);
}
void set_argument(JVMState* jvms, uint idx, Node *c) {
void set_argument(const JVMState* jvms, uint idx, Node *c) {
assert(verify_jvms(jvms), "jvms must match");
set_req(jvms->argoff() + idx, c);
}
Expand Down
7 changes: 7 additions & 0 deletions src/hotspot/share/opto/parse1.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1149,6 +1149,13 @@ SafePointNode* Parse::create_entry_map() {
// Create an initial safepoint to hold JVM state during parsing
JVMState* jvms = new (C) JVMState(method(), _caller->has_method() ? _caller : nullptr);
set_map(new SafePointNode(len, jvms));

// Capture receiver info for compiled lambda forms.
if (method()->is_compiled_lambda_form()) {
ciInstance* recv_info = _caller->compute_receiver_info(method());
jvms->set_receiver_info(recv_info);
}

jvms->set_map(map());
record_for_igvn(map());
assert(jvms->endoff() == len, "correct jvms sizing");
Expand Down