From 7ed3634da81b53135c89aa82a1a5d2f4de2c6c5e Mon Sep 17 00:00:00 2001 From: Vladimir Ivanov Date: Fri, 18 Jun 2021 07:50:22 +0000 Subject: [PATCH] 8268405: Several regressions 4-17% after CHA changes Reviewed-by: kvn, dlong --- src/hotspot/share/opto/c2_globals.hpp | 3 ++ src/hotspot/share/opto/callGenerator.cpp | 8 +++- src/hotspot/share/opto/doCall.cpp | 47 ++++++++++++-------- src/hotspot/share/opto/graphKit.cpp | 4 +- src/hotspot/share/opto/parse1.cpp | 25 ++++------- src/hotspot/share/opto/type.hpp | 2 +- src/hotspot/share/runtime/deoptimization.cpp | 11 ++++- src/hotspot/share/runtime/deoptimization.hpp | 1 + src/hotspot/share/runtime/vmStructs.cpp | 1 + 9 files changed, 61 insertions(+), 41 deletions(-) diff --git a/src/hotspot/share/opto/c2_globals.hpp b/src/hotspot/share/opto/c2_globals.hpp index a029c60d1d7..f6b0ba8da0e 100644 --- a/src/hotspot/share/opto/c2_globals.hpp +++ b/src/hotspot/share/opto/c2_globals.hpp @@ -767,6 +767,9 @@ "to stress handling of long counted loops: run inner loop" \ "for at most jint_max / StressLongCountedLoop") \ range(0, max_juint) \ + \ + product(bool, VerifyReceiverTypes, trueInDebug, DIAGNOSTIC, \ + "Verify receiver types at runtime") \ // end of C2_FLAGS diff --git a/src/hotspot/share/opto/callGenerator.cpp b/src/hotspot/share/opto/callGenerator.cpp index ae05e8b5029..ab7f7897797 100644 --- a/src/hotspot/share/opto/callGenerator.cpp +++ b/src/hotspot/share/opto/callGenerator.cpp @@ -515,6 +515,10 @@ bool LateInlineVirtualCallGenerator::do_late_inline_check(Compile* C, JVMState* // Even if inlining is not allowed, a virtual call can be strength-reduced to a direct call. bool allow_inline = C->inlining_incrementally(); + if (!allow_inline && _callee->holder()->is_interface()) { + // Don't convert the interface call to a direct call guarded by an interface subtype check. + return false; + } CallGenerator* cg = C->call_generator(_callee, vtable_index(), false /*call_does_dispatch*/, @@ -953,12 +957,12 @@ JVMState* PredictedCallGenerator::generate(JVMState* jvms) { } if (kit.stopped()) { - // Instance exactly does not matches the desired type. + // Instance does not match the predicted type. kit.set_jvms(slow_jvms); return kit.transfer_exceptions_into_jvms(); } - // fall through if the instance exactly matches the desired type + // Fall through if the instance matches the desired type. kit.replace_in_map(receiver, casted_receiver); // Make the hot call: diff --git a/src/hotspot/share/opto/doCall.cpp b/src/hotspot/share/opto/doCall.cpp index aacd8ac3275..4a371eb7064 100644 --- a/src/hotspot/share/opto/doCall.cpp +++ b/src/hotspot/share/opto/doCall.cpp @@ -72,6 +72,9 @@ CallGenerator* Compile::call_generator(ciMethod* callee, int vtable_index, bool Bytecodes::Code bytecode = caller->java_code_at_bci(bci); guarantee(callee != NULL, "failed method resolution"); + const bool is_virtual_or_interface = (bytecode == Bytecodes::_invokevirtual) || + (bytecode == Bytecodes::_invokeinterface); + // Dtrace currently doesn't work unless all calls are vanilla if (env()->dtrace_method_probes()) { allow_inline = false; @@ -164,6 +167,18 @@ CallGenerator* Compile::call_generator(ciMethod* callee, int vtable_index, bool bool should_delay = false; if (ilt->ok_to_inline(callee, jvms, profile, should_delay)) { CallGenerator* cg = CallGenerator::for_inline(callee, expected_uses); + // For optimized virtual calls assert at runtime that receiver object + // is a subtype of the inlined method holder. CHA can report a method + // as a unique target under an abstract method, but receiver type + // sometimes has a broader type. Similar scenario is possible with + // default methods when type system loses information about implemented + // interfaces. + if (cg != NULL && is_virtual_or_interface && !callee->is_static()) { + CallGenerator* trap_cg = CallGenerator::for_uncommon_trap(callee, + Deoptimization::Reason_receiver_constraint, Deoptimization::Action_none); + + cg = CallGenerator::for_guarded_call(callee->holder(), trap_cg, cg); + } if (cg != NULL) { // Delay the inlining of this method to give us the // opportunity to perform some high level optimizations @@ -344,9 +359,16 @@ CallGenerator* Compile::call_generator(ciMethod* callee, int vtable_index, bool return CallGenerator::for_virtual_call(callee, vtable_index); } } else { - // Class Hierarchy Analysis or Type Profile reveals a unique target, - // or it is a static or special call. - return CallGenerator::for_direct_call(callee, should_delay_inlining(callee, jvms)); + // Class Hierarchy Analysis or Type Profile reveals a unique target, or it is a static or special call. + CallGenerator* cg = CallGenerator::for_direct_call(callee, should_delay_inlining(callee, jvms)); + // For optimized virtual calls assert at runtime that receiver object + // is a subtype of the method holder. + if (cg != NULL && is_virtual_or_interface && !callee->is_static()) { + CallGenerator* trap_cg = CallGenerator::for_uncommon_trap(callee, + Deoptimization::Reason_receiver_constraint, Deoptimization::Action_none); + cg = CallGenerator::for_guarded_call(callee->holder(), trap_cg, cg); + } + return cg; } } @@ -1105,12 +1127,12 @@ ciMethod* Compile::optimize_inlining(ciMethod* caller, ciInstanceKlass* klass, c return NULL; } - ciInstanceKlass *ikl = receiver_type->klass()->as_instance_klass(); - if (ikl->is_loaded() && ikl->is_initialized() && !ikl->is_interface() && - (ikl == actual_receiver || ikl->is_subtype_of(actual_receiver))) { + ciInstanceKlass* receiver_klass = receiver_type->klass()->as_instance_klass(); + if (receiver_klass->is_loaded() && receiver_klass->is_initialized() && !receiver_klass->is_interface() && + (receiver_klass == actual_receiver || receiver_klass->is_subtype_of(actual_receiver))) { // ikl is a same or better type than the original actual_receiver, // e.g. static receiver from bytecodes. - actual_receiver = ikl; + actual_receiver = receiver_klass; // Is the actual_receiver exact? actual_receiver_is_exact = receiver_type->klass_is_exact(); } @@ -1118,17 +1140,6 @@ ciMethod* Compile::optimize_inlining(ciMethod* caller, ciInstanceKlass* klass, c ciInstanceKlass* calling_klass = caller->holder(); ciMethod* cha_monomorphic_target = callee->find_monomorphic_target(calling_klass, klass, actual_receiver, check_access); - // Validate receiver info against target method. - if (cha_monomorphic_target != NULL) { - bool has_receiver = !cha_monomorphic_target->is_static(); - bool is_interface_holder = cha_monomorphic_target->holder()->is_interface(); - if (has_receiver && !is_interface_holder) { - if (!cha_monomorphic_target->holder()->is_subtype_of(receiver_type->klass())) { - cha_monomorphic_target = NULL; // not a subtype - } - } - } - if (cha_monomorphic_target != NULL) { // Hardwiring a virtual. assert(!callee->can_be_statically_bound(), "should have been handled earlier"); diff --git a/src/hotspot/share/opto/graphKit.cpp b/src/hotspot/share/opto/graphKit.cpp index 9be4238ab6c..c221daabeb0 100644 --- a/src/hotspot/share/opto/graphKit.cpp +++ b/src/hotspot/share/opto/graphKit.cpp @@ -2933,7 +2933,9 @@ Node* Phase::gen_subtype_check(Node* subklass, Node* superklass, Node** ctrl, No } Node* GraphKit::gen_subtype_check(Node* obj_or_subklass, Node* superklass) { - if (ExpandSubTypeCheckAtParseTime) { + bool expand_subtype_check = C->post_loop_opts_phase() || // macro node expansion is over + ExpandSubTypeCheckAtParseTime; // forced expansion + if (expand_subtype_check) { MergeMemNode* mem = merged_memory(); Node* ctrl = control(); Node* subklass = obj_or_subklass; diff --git a/src/hotspot/share/opto/parse1.cpp b/src/hotspot/share/opto/parse1.cpp index 06b37972235..7819812e1fe 100644 --- a/src/hotspot/share/opto/parse1.cpp +++ b/src/hotspot/share/opto/parse1.cpp @@ -1193,17 +1193,22 @@ void Parse::do_method_entry() { make_dtrace_method_entry(method()); } +#ifdef ASSERT // Narrow receiver type when it is too broad for the method being parsed. - ciInstanceKlass* callee_holder = method()->holder(); if (!method()->is_static()) { + ciInstanceKlass* callee_holder = method()->holder(); const Type* holder_type = TypeInstPtr::make(TypePtr::BotPTR, callee_holder); Node* receiver_obj = local(0); const TypeInstPtr* receiver_type = _gvn.type(receiver_obj)->isa_instptr(); if (receiver_type != NULL && !receiver_type->higher_equal(holder_type)) { + // Receiver should always be a subtype of callee holder. + // But, since C2 type system doesn't properly track interfaces, + // the invariant can't be expressed in the type system for default methods. + // Example: for unrelated C <: I and D <: I, (C `meet` D) = Object is_interface(), "missing subtype check"); -#ifdef ASSERT // Perform dynamic receiver subtype check against callee holder class w/ a halt on failure. Node* holder_klass = _gvn.makecon(TypeKlassPtr::make(callee_holder)); Node* not_subtype_ctrl = gen_subtype_check(receiver_obj, holder_klass); @@ -1211,23 +1216,9 @@ void Parse::do_method_entry() { Node* halt = _gvn.transform(new HaltNode(not_subtype_ctrl, frameptr(), "failed receiver subtype check")); C->root()->add_req(halt); -#endif // ASSERT - - // Receiver should always be a subtype of callee holder. - // But, since C2 type system doesn't properly track interfaces, - // the invariant on default methods can't be expressed in the type system. - // Example: for unrelated C <: I and D <: I, (C `meet` D) = Object is_interface()) { - assert(callee_holder->is_subtype_of(receiver_type->klass()), "sanity"); - assert(!receiver_type->klass()->is_interface(), "interface receiver type"); - receiver_type = receiver_type->join_speculative(holder_type)->is_instptr(); // keep speculative part - Node* casted_receiver_obj = _gvn.transform(new CheckCastPPNode(control(), receiver_obj, receiver_type)); - set_local(0, casted_receiver_obj); - } - } } +#endif // ASSERT // If the method is synchronized, we need to construct a lock node, attach // it to the Start node, and pin it there. diff --git a/src/hotspot/share/opto/type.hpp b/src/hotspot/share/opto/type.hpp index 69443137d4a..928920c02c5 100644 --- a/src/hotspot/share/opto/type.hpp +++ b/src/hotspot/share/opto/type.hpp @@ -380,7 +380,7 @@ class Type { Category category() const; static const char* str(const Type* t); -#endif +#endif // !PRODUCT void typerr(const Type *t) const; // Mixing types error // Create basic type diff --git a/src/hotspot/share/runtime/deoptimization.cpp b/src/hotspot/share/runtime/deoptimization.cpp index 5e813386f37..fb5edba1051 100644 --- a/src/hotspot/share/runtime/deoptimization.cpp +++ b/src/hotspot/share/runtime/deoptimization.cpp @@ -1957,7 +1957,9 @@ JRT_ENTRY(void, Deoptimization::uncommon_trap_inner(JavaThread* current, jint tr ScopeDesc* trap_scope = cvf->scope(); - if (TraceDeoptimization) { + bool is_receiver_constraint_failure = VerifyReceiverTypes && (reason == Deoptimization::Reason_receiver_constraint); + + if (TraceDeoptimization || is_receiver_constraint_failure) { ttyLocker ttyl; tty->print_cr(" bci=%d pc=" INTPTR_FORMAT ", relative_pc=" INTPTR_FORMAT ", method=%s" JVMCI_ONLY(", debug_id=%d"), trap_scope->bci(), p2i(fr.pc()), fr.pc() - nm->code_begin(), trap_scope->method()->name_and_sig_as_C_string() #if INCLUDE_JVMCI @@ -2016,7 +2018,7 @@ JRT_ENTRY(void, Deoptimization::uncommon_trap_inner(JavaThread* current, jint tr trap_method->name_and_sig_as_C_string(), trap_bci, nm->compiler_name()); // Print a bunch of diagnostics, if requested. - if (TraceDeoptimization || LogCompilation) { + if (TraceDeoptimization || LogCompilation || is_receiver_constraint_failure) { ResourceMark rm; ttyLocker ttyl; char buf[100]; @@ -2109,6 +2111,10 @@ JRT_ENTRY(void, Deoptimization::uncommon_trap_inner(JavaThread* current, jint tr } // (End diagnostic printout.) + if (is_receiver_constraint_failure) { + fatal("missing receiver type check"); + } + // Load class if necessary if (unloaded_class_index >= 0) { constantPoolHandle constants(current, trap_method->constants()); @@ -2598,6 +2604,7 @@ const char* Deoptimization::_trap_reason_name[] = { "rtm_state_change", "unstable_if", "unstable_fused_if", + "receiver_constraint", #if INCLUDE_JVMCI "aliasing", "transfer_to_interpreter", diff --git a/src/hotspot/share/runtime/deoptimization.hpp b/src/hotspot/share/runtime/deoptimization.hpp index 8a41a50a47f..20c9e6b3641 100644 --- a/src/hotspot/share/runtime/deoptimization.hpp +++ b/src/hotspot/share/runtime/deoptimization.hpp @@ -89,6 +89,7 @@ class Deoptimization : AllStatic { Reason_rtm_state_change, // rtm state change detected Reason_unstable_if, // a branch predicted always false was taken Reason_unstable_fused_if, // fused two ifs that had each one untaken branch. One is now taken. + Reason_receiver_constraint, // receiver subtype check failed #if INCLUDE_JVMCI Reason_aliasing, // optimistic assumption about aliasing failed Reason_transfer_to_interpreter, // explicit transferToInterpreter() diff --git a/src/hotspot/share/runtime/vmStructs.cpp b/src/hotspot/share/runtime/vmStructs.cpp index 265bbf1d290..2a38e09e67f 100644 --- a/src/hotspot/share/runtime/vmStructs.cpp +++ b/src/hotspot/share/runtime/vmStructs.cpp @@ -2402,6 +2402,7 @@ typedef HashtableEntry KlassHashtableEntry; declare_constant(Deoptimization::Reason_rtm_state_change) \ declare_constant(Deoptimization::Reason_unstable_if) \ declare_constant(Deoptimization::Reason_unstable_fused_if) \ + declare_constant(Deoptimization::Reason_receiver_constraint) \ NOT_ZERO(JVMCI_ONLY(declare_constant(Deoptimization::Reason_aliasing))) \ NOT_ZERO(JVMCI_ONLY(declare_constant(Deoptimization::Reason_transfer_to_interpreter))) \ NOT_ZERO(JVMCI_ONLY(declare_constant(Deoptimization::Reason_not_compiled_exception_handler))) \