--- old/src/hotspot/share/c1/c1_GraphBuilder.cpp 2019-05-10 14:05:56.203456843 -0700 +++ new/src/hotspot/share/c1/c1_GraphBuilder.cpp 2019-05-10 14:05:55.931447001 -0700 @@ -1241,9 +1241,39 @@ BlockBegin* tsux = block_at(stream()->get_dest()); BlockBegin* fsux = block_at(stream()->next_bci()); bool is_bb = tsux->bci() < stream()->cur_bci() || fsux->bci() < stream()->cur_bci(); + + bool subst_check = false; + if (EnableValhalla && ACmpOnValues == 3 && + (stream()->cur_bc() == Bytecodes::_if_acmpeq || stream()->cur_bc() == Bytecodes::_if_acmpne) && + method() != ciEnv::current()->ValueBootstrapMethods_klass()->find_method(ciSymbol::isSubstitutable_name(), ciSymbol::object_object_boolean_signature())) { + // If current method is ValueBootstrapMethods::isSubstitutable(), + // compile the acmp as a regular pointer comparison otherwise we + // could call ValueBootstrapMethods::isSubstitutable() back + ValueType* left_vt = x->type(); + ValueType* right_vt = y->type(); + if (left_vt->is_object()) { + assert(right_vt->is_object(), "must be"); + ciKlass* left_klass = x->as_loaded_klass_or_null(); + ciKlass* right_klass = y->as_loaded_klass_or_null(); + + if (left_klass == NULL || right_klass == NULL) { + // The klass is still unloaded, or came from a Phi node. Go slow case; + subst_check = true; + } else if (left_klass->is_java_lang_Object() || left_klass->is_interface() || + right_klass->is_java_lang_Object() || right_klass->is_interface()) { + // Either operand may be a value object, but we're not sure. Go slow case; + subst_check = true; + } else if (left_klass->is_valuetype() || right_klass->is_valuetype()) { + subst_check = true; + } else { + // No need to do substituability check + } + } + } + // In case of loop invariant code motion or predicate insertion // before the body of a loop the state is needed - Instruction *i = append(new If(x, cond, false, y, tsux, fsux, (is_bb || compilation()->is_optimistic()) ? state_before : NULL, is_bb)); + Instruction *i = append(new If(x, cond, false, y, tsux, fsux, (is_bb || compilation()->is_optimistic() || subst_check) ? state_before : NULL, is_bb, subst_check)); assert(i->as_Goto() == NULL || (i->as_Goto()->sux_at(0) == tsux && i->as_Goto()->is_safepoint() == tsux->bci() < stream()->cur_bci()) || --- old/src/hotspot/share/c1/c1_Instruction.cpp 2019-05-10 14:05:56.895481881 -0700 +++ new/src/hotspot/share/c1/c1_Instruction.cpp 2019-05-10 14:05:56.623472039 -0700 @@ -114,6 +114,16 @@ return NULL; } +ciKlass* Instruction::as_loaded_klass_or_null() const { + ciType* type = declared_type(); + if (type != NULL && type->is_klass()) { + ciKlass* klass = type->as_klass(); + if (klass->is_loaded()) { + return klass; + } + } + return NULL; +} // FIXME -- this is used by ValueStack::merge_types only. We should remove this function // and use a better way for handling phi nodes. --- old/src/hotspot/share/c1/c1_Instruction.hpp 2019-05-10 14:05:57.559505906 -0700 +++ new/src/hotspot/share/c1/c1_Instruction.hpp 2019-05-10 14:05:57.287496065 -0700 @@ -464,6 +464,7 @@ ValueStack* exception_state() const { return _exception_state; } virtual bool needs_exception_state() const { return true; } XHandlers* exception_handlers() const { return _exception_handlers; } + ciKlass* as_loaded_klass_or_null() const; // manipulation void pin(PinReason reason) { _pin_state |= reason; } @@ -1145,16 +1146,19 @@ private: Value _tval; Value _fval; + bool _substituability_check; public: // creation - IfOp(Value x, Condition cond, Value y, Value tval, Value fval) + IfOp(Value x, Condition cond, Value y, Value tval, Value fval, ValueStack* state_before, bool substituability_check) : Op2(tval->type()->meet(fval->type()), (Bytecodes::Code)cond, x, y) , _tval(tval) , _fval(fval) + , _substituability_check(substituability_check) { ASSERT_VALUES assert(tval->type()->tag() == fval->type()->tag(), "types must match"); + set_state_before(state_before); } // accessors @@ -1163,7 +1167,7 @@ Condition cond() const { return (Condition)Op2::op(); } Value tval() const { return _tval; } Value fval() const { return _fval; } - + bool substituability_check() const { return _substituability_check; } // generic virtual void input_values_do(ValueVisitor* f) { Op2::input_values_do(f); f->visit(&_tval); f->visit(&_fval); } }; @@ -2054,10 +2058,11 @@ int _profiled_bci; // Canonicalizer may alter bci of If node bool _swapped; // Is the order reversed with respect to the original If in the // bytecode stream? + bool _substituability_check; public: // creation // unordered_is_true is valid for float/double compares only - If(Value x, Condition cond, bool unordered_is_true, Value y, BlockBegin* tsux, BlockBegin* fsux, ValueStack* state_before, bool is_safepoint) + If(Value x, Condition cond, bool unordered_is_true, Value y, BlockBegin* tsux, BlockBegin* fsux, ValueStack* state_before, bool is_safepoint, bool substituability_check=false) : BlockEnd(illegalType, state_before, is_safepoint) , _x(x) , _cond(cond) @@ -2065,6 +2070,7 @@ , _profiled_method(NULL) , _profiled_bci(0) , _swapped(false) + , _substituability_check(substituability_check) { ASSERT_VALUES set_flag(UnorderedIsTrueFlag, unordered_is_true); @@ -2107,6 +2113,7 @@ void set_profiled_method(ciMethod* method) { _profiled_method = method; } void set_profiled_bci(int bci) { _profiled_bci = bci; } void set_swapped(bool value) { _swapped = value; } + bool substituability_check() const { return _substituability_check; } // generic virtual void input_values_do(ValueVisitor* f) { BlockEnd::input_values_do(f); f->visit(&_x); f->visit(&_y); } }; --- old/src/hotspot/share/c1/c1_LIRGenerator.cpp 2019-05-10 14:05:58.239530510 -0700 +++ new/src/hotspot/share/c1/c1_LIRGenerator.cpp 2019-05-10 14:05:57.967520669 -0700 @@ -3001,6 +3001,24 @@ __ move(LIR_Assembler::osrBufferPointer(), result); } +void LIRGenerator::invoke_load_one_argument(LIRItem* param, LIR_Opr loc) { + if (loc->is_register()) { + param->load_item_force(loc); + } else { + LIR_Address* addr = loc->as_address_ptr(); + param->load_for_store(addr->type()); + assert(addr->type() != T_VALUETYPE, "not supported yet"); + if (addr->type() == T_OBJECT) { + __ move_wide(param->result(), addr); + } else { + if (addr->type() == T_LONG || addr->type() == T_DOUBLE) { + __ unaligned_move(param->result(), addr); + } else { + __ move(param->result(), addr); + } + } + } +} void LIRGenerator::invoke_load_arguments(Invoke* x, LIRItemList* args, const LIR_OprList* arg_list) { assert(args->length() == arg_list->length(), @@ -3008,21 +3026,7 @@ for (int i = x->has_receiver() ? 1 : 0; i < args->length(); i++) { LIRItem* param = args->at(i); LIR_Opr loc = arg_list->at(i); - if (loc->is_register()) { - param->load_item_force(loc); - } else { - LIR_Address* addr = loc->as_address_ptr(); - param->load_for_store(addr->type()); - assert(addr->type() != T_VALUETYPE, "not supported yet"); - if (addr->type() == T_OBJECT) { - __ move_wide(param->result(), addr); - } else - if (addr->type() == T_LONG || addr->type() == T_DOUBLE) { - __ unaligned_move(param->result(), addr); - } else { - __ move(param->result(), addr); - } - } + invoke_load_one_argument(param, loc); } if (x->has_receiver()) { @@ -3203,9 +3207,10 @@ LIRItem left(x->x(), this); LIRItem right(x->y(), this); left.load_item(); - if (can_inline_as_constant(right.value())) { + if (can_inline_as_constant(right.value()) && !x->substituability_check()) { right.dont_load_item(); } else { + // substituability_check() needs to use right as a base register. right.load_item(); } @@ -3213,10 +3218,134 @@ LIRItem f_val(x->fval(), this); t_val.dont_load_item(); f_val.dont_load_item(); + + if (x->substituability_check()) { + substituability_check(x, left, right, t_val, f_val); + } else { + LIR_Opr reg = rlock_result(x); + __ cmp(lir_cond(x->cond()), left.result(), right.result()); + __ cmove(lir_cond(x->cond()), t_val.result(), f_val.result(), reg, as_BasicType(x->x()->type())); + } +} + +void LIRGenerator::substituability_check(IfOp* x, LIRItem& left, LIRItem& right, LIRItem& t_val, LIRItem& f_val) { + assert(x->cond() == If::eql || x->cond() == If::neq, "must be"); + bool is_acmpeq = (x->cond() == If::eql); LIR_Opr reg = rlock_result(x); + LIR_Opr equal_result = is_acmpeq ? t_val.result() : f_val.result(); + LIR_Opr not_equal_result = is_acmpeq ? f_val.result() : t_val.result(); + + LabelObj* L_oops_equal = new LabelObj(); + LabelObj* L_oops_not_equal = new LabelObj(); + LabelObj* L_do_subst_check = new LabelObj(); + LabelObj* L_end = new LabelObj(); + + __ cmp(lir_cond_equal, left.result(), right.result()); + __ branch(lir_cond_equal, T_ILLEGAL, L_oops_equal->label()); + + // The two operands are not the same reference. Do a more costly substituability check. + + ciKlass* left_klass = x->x()->as_loaded_klass_or_null(); + ciKlass* right_klass = x->y()->as_loaded_klass_or_null(); + + // (1) Null check -- if one of the operands is null, the other must not be null (because + // the two references are not equal), so they are not substitutable, + // FIXME: do null check only if the operand is nullable + { + __ cmp(lir_cond_equal, left.result(), LIR_OprFact::oopConst(NULL)); + __ branch(lir_cond_equal, T_ILLEGAL, L_oops_not_equal->label()); + + __ cmp(lir_cond_equal, right.result(), LIR_OprFact::oopConst(NULL)); + __ branch(lir_cond_equal, T_ILLEGAL, L_oops_not_equal->label()); + } + + // (2) Value object check -- if either of the operands is not a value object, + // they are not substitutable. We do this only if we are not sure that the + // operands value objects + if ((left_klass == NULL || right_klass == NULL) ||// The klass is still unloaded, or came from a Phi node. + !left_klass->is_valuetype() || !right_klass->is_valuetype()) { + // FIXME: on x64, this can be optimized as: + // mov $0x405,%r10d + // and (%left), %r10d /* if need to check left */ + // and (%right), %r10d /* if need to check right */ + // cmp $0x405, $r10d + // jne L_oops_not_equal + + LIR_Opr mark = new_register(T_LONG); + LIR_Opr always_locked_pattern = new_register(T_LONG); + __ move(LIR_OprFact::longConst(markOopDesc::always_locked_pattern), always_locked_pattern); + + if (left_klass == NULL || !left_klass->is_valuetype()) { + __ move(new LIR_Address(left.result(), oopDesc::mark_offset_in_bytes(), T_LONG), mark); + __ logical_and(mark, always_locked_pattern, mark); + __ cmp(lir_cond_notEqual, mark, always_locked_pattern); + __ branch(lir_cond_notEqual, T_ILLEGAL, L_oops_not_equal->label()); + } + + if (right_klass == NULL || !right_klass->is_valuetype()) { + __ move(new LIR_Address(right.result(), oopDesc::mark_offset_in_bytes(), T_LONG), mark); + __ logical_and(mark, always_locked_pattern, mark); + __ cmp(lir_cond_notEqual, mark, always_locked_pattern); + __ branch(lir_cond_notEqual, T_ILLEGAL, L_oops_not_equal->label()); + } + } + + // (3) Same klass check: if the operands are of different klasses, they are not substitutable. + if (left_klass != NULL && left_klass->is_valuetype() && left_klass == right_klass) { + // No need to check -- they are known to be the same value klass. + __ branch(lir_cond_always, T_ILLEGAL, L_do_subst_check->label()); + } else { + BasicType t_klass = UseCompressedOops ? T_INT : T_METADATA; + BasicType t_addr = UseCompressedOops ? T_INT : T_ADDRESS; + + LIR_Opr left_klass = new_register(t_klass); + LIR_Opr right_klass = new_register(t_klass); + __ move(new LIR_Address(left.result(), oopDesc::klass_offset_in_bytes(), t_addr), left_klass); + __ move(new LIR_Address(right.result(), oopDesc::klass_offset_in_bytes(), t_addr), right_klass); + __ cmp(lir_cond_equal, left_klass, right_klass); + __ branch(lir_cond_equal, T_ILLEGAL, L_do_subst_check->label()); + + // fall through to L_oops_not_equal + } + + __ branch_destination(L_oops_not_equal->label()); + __ move(not_equal_result, reg); + __ branch(lir_cond_always, T_ILLEGAL, L_end->label()); + + + // FIXME -- for simple case (no non-flattened value fields), do a per-field comparison + + __ branch_destination(L_do_subst_check->label()); + { + // Call into ValueBootstrapMethods::isSubstitutable() + ciMethod* subst_method = ciEnv::current()->ValueBootstrapMethods_klass()->find_method(ciSymbol::isSubstitutable_name(), ciSymbol::object_object_boolean_signature()); + assert(method() != subst_method, "cannot recurse!"); + + const LIR_Opr result = result_register_for(x->type()); + CodeEmitInfo* info = state_for(x, x->state_before()); + BasicTypeList signature(2); + signature.append(T_OBJECT); + signature.append(T_OBJECT); + + CallingConvention* cc = frame_map()->java_calling_convention(&signature, true); + LIR_OprList* arg_list = cc->args(); + + left.set_destroys_register(); + right.set_destroys_register(); + invoke_load_one_argument(&left, arg_list->at(0)); + invoke_load_one_argument(&right, arg_list->at(1)); + __ call_static(subst_method, result, + SharedRuntime::get_resolve_static_call_stub(), + arg_list, info); + __ cmp(lir_cond_notEqual, result, LIR_OprFact::intConst(1)); + } + __ move(not_equal_result, reg); + __ branch(lir_cond_notEqual, T_ILLEGAL, L_end->label()); + + __ branch_destination(L_oops_equal->label()); + __ move(equal_result, reg); - __ cmp(lir_cond(x->cond()), left.result(), right.result()); - __ cmove(lir_cond(x->cond()), t_val.result(), f_val.result(), reg, as_BasicType(x->x()->type())); + __ branch_destination(L_end->label()); } #ifdef JFR_HAVE_INTRINSICS --- old/src/hotspot/share/c1/c1_LIRGenerator.hpp 2019-05-10 14:05:58.935555693 -0700 +++ new/src/hotspot/share/c1/c1_LIRGenerator.hpp 2019-05-10 14:05:58.663545851 -0700 @@ -270,6 +270,7 @@ void access_flattened_array(bool is_load, LIRItem& array, LIRItem& index, LIRItem& obj_item); bool needs_flattened_array_store_check(StoreIndexed* x); void check_flattened_array(LIRItem& array, CodeStub* slow_path); + void substituability_check(IfOp* x, LIRItem& left, LIRItem& right, LIRItem& t_val, LIRItem& f_val); public: LIR_Opr call_runtime(BasicTypeArray* signature, LIRItemList* args, address entry, ValueType* result_type, CodeEmitInfo* info); @@ -327,7 +328,7 @@ LIRItemList* invoke_visit_arguments(Invoke* x); void invoke_load_arguments(Invoke* x, LIRItemList* args, const LIR_OprList* arg_list); - + void invoke_load_one_argument(LIRItem* param, LIR_Opr loc); void trace_block_entry(BlockBegin* block); // volatile field operations are never patchable because a klass --- old/src/hotspot/share/c1/c1_Optimizer.cpp 2019-05-10 14:05:59.591579429 -0700 +++ new/src/hotspot/share/c1/c1_Optimizer.cpp 2019-05-10 14:05:59.319569587 -0700 @@ -1,5 +1,5 @@ /* - * Copyright (c) 1999, 2017, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1999, 2019, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -89,7 +89,8 @@ virtual void block_do(BlockBegin* block); private: - Value make_ifop(Value x, Instruction::Condition cond, Value y, Value tval, Value fval); + Value make_ifop(Value x, Instruction::Condition cond, Value y, Value tval, Value fval, + ValueStack* state_before, bool substituability_check); }; void CE_Eliminator::block_do(BlockBegin* block) { @@ -199,7 +200,8 @@ cur_end = cur_end->set_next(f_value); } - Value result = make_ifop(if_->x(), if_->cond(), if_->y(), t_value, f_value); + Value result = make_ifop(if_->x(), if_->cond(), if_->y(), t_value, f_value, + if_->state_before(), if_->substituability_check()); assert(result != NULL, "make_ifop must return a non-null instruction"); if (!result->is_linked() && result->can_be_linked()) { NOT_PRODUCT(result->set_printable_bci(if_->printable_bci())); @@ -251,9 +253,10 @@ _hir->verify(); } -Value CE_Eliminator::make_ifop(Value x, Instruction::Condition cond, Value y, Value tval, Value fval) { +Value CE_Eliminator::make_ifop(Value x, Instruction::Condition cond, Value y, Value tval, Value fval, + ValueStack* state_before, bool substituability_check) { if (!OptimizeIfOps) { - return new IfOp(x, cond, y, tval, fval); + return new IfOp(x, cond, y, tval, fval, state_before, substituability_check); } tval = tval->subst(); @@ -288,7 +291,7 @@ if (new_tval == new_fval) { return new_tval; } else { - return new IfOp(x_ifop->x(), x_ifop_cond, x_ifop->y(), new_tval, new_fval); + return new IfOp(x_ifop->x(), x_ifop_cond, x_ifop->y(), new_tval, new_fval, state_before, substituability_check); } } } @@ -304,7 +307,7 @@ } } } - return new IfOp(x, cond, y, tval, fval); + return new IfOp(x, cond, y, tval, fval, state_before, substituability_check); } void Optimizer::eliminate_conditional_expressions() { @@ -436,7 +439,7 @@ BlockBegin* fblock = fval->compare(cond, con, tsux, fsux); if (tblock != fblock && !if_->is_safepoint()) { If* newif = new If(ifop->x(), ifop->cond(), false, ifop->y(), - tblock, fblock, if_->state_before(), if_->is_safepoint()); + tblock, fblock, if_->state_before(), if_->is_safepoint(), if_->substituability_check()); newif->set_state(if_->state()->copy()); assert(prev->next() == if_, "must be guaranteed by above search"); --- old/test/hotspot/jtreg/compiler/valhalla/valuetypes/TestNewAcmp.java 2019-05-10 14:06:00.267603888 -0700 +++ new/test/hotspot/jtreg/compiler/valhalla/valuetypes/TestNewAcmp.java 2019-05-10 14:05:59.983593612 -0700 @@ -1388,10 +1388,21 @@ return a != a; } + static int get_full_opt_level() { + int n = (int)TieredStopAtLevel; + if (n >= 4) { + n = 4; + } + return n; + } protected static final WhiteBox WHITE_BOX = WhiteBox.getWhiteBox(); - protected static final int COMP_LEVEL_FULL_OPTIMIZATION = 4; + protected static final long TieredStopAtLevel = (Long)WHITE_BOX.getVMFlag("TieredStopAtLevel"); + protected static final int COMP_LEVEL_FULL_OPTIMIZATION = get_full_opt_level(); protected static final long ACmpOnValues = (Long)WHITE_BOX.getVMFlag("ACmpOnValues"); + // FIXME: temp -- special handling for C1 testing. + protected static final boolean EnableValhallaC1 = (Boolean)WHITE_BOX.getVMFlag("EnableValhallaC1"); + public void runTest(Method m, Object[] args, int warmup, int nullMode, boolean[][] equalities) throws Exception { Class[] parameterTypes = m.getParameterTypes(); int parameterCount = parameterTypes.length; @@ -1528,11 +1539,6 @@ } public static void main(String[] args) throws Exception { - if (Boolean.getBoolean("test.c1")) { - System.out.println("new acmp is not implemented for C1"); // TMP - return; - } - if (args.length == 0) { enumerateVMOptions(); } else { @@ -1581,13 +1587,8 @@ for (int onVal = 0; onVal < 2; onVal++) { // 0 = default, 1 = -XX:ACmpOnValues=3 for (int incrInline = 0; incrInline < 2; incrInline++) { // 0 = default, 1 = -XX:+AlwaysIncrementalInline scenario++; - if (scenarios != null && !scenarios.contains(Integer.toString(scenario))) { - System.out.println("Scenario #" + scenario + " is skipped due to -Dscenarios=" + SCENARIOS); - continue; - } else { - System.out.println("Scenario #" + scenario + " -------------------"); - } - + + System.out.println("\nScenario #" + scenario + " -------------------"); String[] cmds = baseOptions; if (incrInline != 0) { cmds = addOptions(cmds, "-XX:+AlwaysIncrementalInline"); @@ -1595,9 +1596,23 @@ if (onVal != 0) { cmds = addOptions(cmds, "-XX:+UnlockExperimentalVMOptions", "-XX:ACmpOnValues=3"); } + + if (EnableValhallaC1) { + // FIXME: C1 runs into problems when these methods are compiled + cmds = addOptions(cmds, "-XX:CompileCommand=exclude,java.lang.ClassValue::*"); + } + cmds = addOptions(cmds, "compiler.valhalla.valuetypes.TestNewAcmp"); cmds = addOptions(cmds, Integer.toString(nullMode)); + if (scenarios != null && !scenarios.contains(Integer.toString(scenario))) { + System.out.println("Scenario #" + scenario + " is skipped due to -Dscenarios=" + SCENARIOS); + continue; + } else if (EnableValhallaC1 && onVal == 0) { + System.out.println("Scenario #" + scenario + " is skipped because C1 requires -XX:ACmpOnValues=3"); + continue; + } + OutputAnalyzer oa = ProcessTools.executeTestJvm(cmds); String output = oa.getOutput(); oa.shouldHaveExitValue(0);