diff --git a/compiler/optimizing/graph_checker.cc b/compiler/optimizing/graph_checker.cc index e4203fe492..3d80de0e04 100644 --- a/compiler/optimizing/graph_checker.cc +++ b/compiler/optimizing/graph_checker.cc @@ -1064,6 +1064,21 @@ void GraphChecker::VisitNeg(HNeg* instruction) { } } +void GraphChecker::VisitArraySet(HArraySet* instruction) { + VisitInstruction(instruction); + + if (instruction->NeedsTypeCheck() != + instruction->GetSideEffects().Includes(SideEffects::CanTriggerGC())) { + AddError(StringPrintf( + "%s %d has a flag mismatch. An ArraySet instruction can trigger a GC iff it " + "needs a type check. Needs type check: %s, Can trigger GC: %s", + instruction->DebugName(), + instruction->GetId(), + instruction->NeedsTypeCheck() ? "true" : "false", + instruction->GetSideEffects().Includes(SideEffects::CanTriggerGC()) ? "true" : "false")); + } +} + void GraphChecker::VisitBinaryOperation(HBinaryOperation* op) { VisitInstruction(op); DataType::Type lhs_type = op->InputAt(0)->GetType(); diff --git a/compiler/optimizing/graph_checker.h b/compiler/optimizing/graph_checker.h index c3cdba5d42..950868b4ba 100644 --- a/compiler/optimizing/graph_checker.h +++ b/compiler/optimizing/graph_checker.h @@ -55,6 +55,7 @@ class GraphChecker : public HGraphDelegateVisitor { void VisitInstruction(HInstruction* instruction) override; void VisitPhi(HPhi* phi) override; + void VisitArraySet(HArraySet* instruction) override; void VisitBinaryOperation(HBinaryOperation* op) override; void VisitBooleanNot(HBooleanNot* instruction) override; void VisitBoundType(HBoundType* instruction) override; diff --git a/compiler/optimizing/graph_visualizer.cc b/compiler/optimizing/graph_visualizer.cc index 646c34e384..f6076525d8 100644 --- a/compiler/optimizing/graph_visualizer.cc +++ b/compiler/optimizing/graph_visualizer.cc @@ -480,6 +480,9 @@ class HGraphVisualizerPrinter : public HGraphDelegateVisitor { << array_set->GetValueCanBeNull() << std::noboolalpha; StartAttributeStream("needs_type_check") << std::boolalpha << array_set->NeedsTypeCheck() << std::noboolalpha; + StartAttributeStream("can_trigger_gc") + << std::boolalpha << array_set->GetSideEffects().Includes(SideEffects::CanTriggerGC()) + << std::noboolalpha; } void VisitCompare(HCompare* compare) override { diff --git a/compiler/optimizing/instruction_simplifier.cc b/compiler/optimizing/instruction_simplifier.cc index ac212e07fe..82c1f6d3ff 100644 --- a/compiler/optimizing/instruction_simplifier.cc +++ b/compiler/optimizing/instruction_simplifier.cc @@ -1142,13 +1142,13 @@ void InstructionSimplifierVisitor::VisitArraySet(HArraySet* instruction) { if (value->IsArrayGet()) { if (value->AsArrayGet()->GetArray() == instruction->GetArray()) { // If the code is just swapping elements in the array, no need for a type check. - instruction->ClearNeedsTypeCheck(); + instruction->ClearTypeCheck(); return; } } if (value->IsNullConstant()) { - instruction->ClearNeedsTypeCheck(); + instruction->ClearTypeCheck(); return; } @@ -1160,13 +1160,13 @@ void InstructionSimplifierVisitor::VisitArraySet(HArraySet* instruction) { } if (value_rti.IsValid() && array_rti.CanArrayHold(value_rti)) { - instruction->ClearNeedsTypeCheck(); + instruction->ClearTypeCheck(); return; } if (array_rti.IsObjectArray()) { if (array_rti.IsExact()) { - instruction->ClearNeedsTypeCheck(); + instruction->ClearTypeCheck(); return; } instruction->SetStaticTypeOfArrayIsObjectArray(); diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h index 17ede39d1e..006ccfb8ec 100644 --- a/compiler/optimizing/nodes.h +++ b/compiler/optimizing/nodes.h @@ -6597,8 +6597,10 @@ class HArraySet final : public HExpression<3> { return false; } - void ClearNeedsTypeCheck() { + void ClearTypeCheck() { SetPackedFlag(false); + // Clear the `CanTriggerGC` flag too as we can only trigger a GC when doing a type check. + SetSideEffects(GetSideEffects().Exclusion(SideEffects::CanTriggerGC())); } void ClearValueCanBeNull() { diff --git a/compiler/optimizing/prepare_for_register_allocation.cc b/compiler/optimizing/prepare_for_register_allocation.cc index 5ce63284ae..707783e47b 100644 --- a/compiler/optimizing/prepare_for_register_allocation.cc +++ b/compiler/optimizing/prepare_for_register_allocation.cc @@ -109,7 +109,7 @@ void PrepareForRegisterAllocation::VisitArraySet(HArraySet* instruction) { if (value->IsNullConstant()) { DCHECK_EQ(value->GetType(), DataType::Type::kReference); if (instruction->NeedsTypeCheck()) { - instruction->ClearNeedsTypeCheck(); + instruction->ClearTypeCheck(); } } } diff --git a/test/521-checker-array-set-null/src/Main.java b/test/521-checker-array-set-null/src/Main.java index f166b92fa4..12a1985bfa 100644 --- a/test/521-checker-array-set-null/src/Main.java +++ b/test/521-checker-array-set-null/src/Main.java @@ -16,26 +16,64 @@ public class Main { public static void main(String[] args) { - testWithNull(new Object[2]); - testWithUnknown(new Object[2], new Object()); - testWithSame(new Object[2]); + $noinline$testWithNull(new Object[2]); + $noinline$testWithUnknown(new Object[2], new Object()); + $noinline$testWithSame(new Object[2]); + $noinline$testWithSameRTI(); } - /// CHECK-START: void Main.testWithNull(java.lang.Object[]) disassembly (after) - /// CHECK: ArraySet needs_type_check:false - public static void testWithNull(Object[] o) { + // Known null can eliminate the type check in early stages. + + /// CHECK-START: void Main.$noinline$testWithNull(java.lang.Object[]) instruction_simplifier (before) + /// CHECK: ArraySet needs_type_check:true can_trigger_gc:true + + /// CHECK-START: void Main.$noinline$testWithNull(java.lang.Object[]) instruction_simplifier (after) + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false + + /// CHECK-START: void Main.$noinline$testWithNull(java.lang.Object[]) disassembly (after) + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false + public static void $noinline$testWithNull(Object[] o) { o[0] = null; } - /// CHECK-START: void Main.testWithUnknown(java.lang.Object[], java.lang.Object) disassembly (after) - /// CHECK: ArraySet needs_type_check:true - public static void testWithUnknown(Object[] o, Object obj) { + /// CHECK-START: void Main.$noinline$testWithUnknown(java.lang.Object[], java.lang.Object) disassembly (after) + /// CHECK: ArraySet needs_type_check:true can_trigger_gc:true + public static void $noinline$testWithUnknown(Object[] o, Object obj) { o[0] = obj; } - /// CHECK-START: void Main.testWithSame(java.lang.Object[]) disassembly (after) - /// CHECK: ArraySet needs_type_check:false - public static void testWithSame(Object[] o) { + // After GVN we know that we are setting values from the same array so there's no need for a type + // check. + + /// CHECK-START: void Main.$noinline$testWithSame(java.lang.Object[]) instruction_simplifier$after_gvn (before) + /// CHECK: ArraySet needs_type_check:true can_trigger_gc:true + + /// CHECK-START: void Main.$noinline$testWithSame(java.lang.Object[]) instruction_simplifier$after_gvn (after) + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false + + /// CHECK-START: void Main.$noinline$testWithSame(java.lang.Object[]) disassembly (after) + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false + public static void $noinline$testWithSame(Object[] o) { o[0] = o[1]; } + + // We know that the array and the static Object have the same RTI in early stages. No need for a + // type check. + + /// CHECK-START: java.lang.Object[] Main.$noinline$testWithSameRTI() instruction_simplifier (before) + /// CHECK: ArraySet needs_type_check:true can_trigger_gc:true + + /// CHECK-START: java.lang.Object[] Main.$noinline$testWithSameRTI() instruction_simplifier (after) + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false + + /// CHECK-START: java.lang.Object[] Main.$noinline$testWithSameRTI() disassembly (after) + /// CHECK: ArraySet needs_type_check:false can_trigger_gc:false + public static Object[] $noinline$testWithSameRTI() { + Object[] arr = new Object[1]; + arr[0] = static_obj; + // Return so that LSE doesn't eliminate the ArraySet. + return arr; + } + + static Object static_obj; } diff --git a/test/527-checker-array-access-split/src/Main.java b/test/527-checker-array-access-split/src/Main.java index f39b5e26a0..cc1dc6b8fc 100644 --- a/test/527-checker-array-access-split/src/Main.java +++ b/test/527-checker-array-access-split/src/Main.java @@ -593,12 +593,16 @@ public final static void checkGVNForFatalChecks(int begin, int end, char[] buf1, /// CHECK: ArrayGet [<>,{{i\d+}}] /// CHECK: <> IntermediateAddress [<>,<>] /// CHECK: ArrayGet [<>,{{i\d+}}] - /// CHECK: ArraySet [<>,{{i\d+}},{{l\d+}}] + /// CHECK: ArraySet [<>,{{i\d+}},{{l\d+}}] needs_type_check:false can_trigger_gc:false /// CHECK: <> IntermediateAddress [<>,<>] /// CHECK: ArrayGet [<>,{{i\d+}}] /// CHECK: ArraySet [<>,{{i\d+}},{{l\d+}}] /// CHECK: ArraySet [<>,{{i\d+}},{{l\d+}}] // + /// CHECK-START-ARM64: int Main.checkObjectArrayGet(int, java.lang.Integer[], java.lang.Integer[]) instruction_simplifier_arm64 (after) + /// CHECK: IntermediateAddress + /// CHECK: IntermediateAddress + /// CHECK: IntermediateAddress /// CHECK-NOT: IntermediateAddress /// CHECK-START-ARM64: int Main.checkObjectArrayGet(int, java.lang.Integer[], java.lang.Integer[]) GVN$after_arch (after) @@ -608,12 +612,13 @@ public final static void checkGVNForFatalChecks(int begin, int end, char[] buf1, /// CHECK: <> IntermediateAddress [<>,<>] /// CHECK: ArrayGet [<>,{{i\d+}}] /// CHECK: ArrayGet [<>,{{i\d+}}] - /// CHECK: ArraySet [<>,{{i\d+}},{{l\d+}}] - /// CHECK: <> IntermediateAddress [<>,<>] - /// CHECK: ArrayGet [<>,{{i\d+}}] + /// CHECK: ArraySet [<>,{{i\d+}},{{l\d+}}] needs_type_check:false can_trigger_gc:false + /// CHECK: ArrayGet [<>,{{i\d+}}] /// CHECK: ArraySet [<>,{{i\d+}},{{l\d+}}] /// CHECK: ArraySet [<>,{{i\d+}},{{l\d+}}] // + /// CHECK-START-ARM64: int Main.checkObjectArrayGet(int, java.lang.Integer[], java.lang.Integer[]) GVN$after_arch (after) + /// CHECK: IntermediateAddress /// CHECK-NOT: IntermediateAddress public final static int checkObjectArrayGet(int index, Integer[] a, Integer[] b) { Integer five = Integer.valueOf(5); diff --git a/test/590-checker-arr-set-null-regression/src/Main.java b/test/590-checker-arr-set-null-regression/src/Main.java index 792ee4ecd6..ad47716c40 100644 --- a/test/590-checker-arr-set-null-regression/src/Main.java +++ b/test/590-checker-arr-set-null-regression/src/Main.java @@ -33,7 +33,7 @@ public static void main(String args[]) { /// CHECK-DAG: <> NullCheck [<>] /// CHECK-DAG: <> ArrayLength [<>] /// CHECK-DAG: <> BoundsCheck [<>,<>] - /// CHECK-DAG: <> ArraySet [<>,<>,<>] needs_type_check:true + /// CHECK-DAG: <> ArraySet [<>,<>,<>] needs_type_check:true can_trigger_gc:true /// CHECK-START: void Main.testArraySetCheckCastNull(Main$Element[]) instruction_simplifier (after) /// CHECK-NOT: CheckCast @@ -47,7 +47,7 @@ public static void main(String args[]) { /// CHECK-DAG: <> NullCheck [<>] /// CHECK-DAG: <> ArrayLength [<>] /// CHECK-DAG: <> BoundsCheck [<>,<>] - /// CHECK-DAG: <> ArraySet [<>,<>,<>] needs_type_check:true + /// CHECK-DAG: <> ArraySet [<>,<>,<>] needs_type_check:true can_trigger_gc:true /// CHECK-START: void Main.testArraySetCheckCastNull(Main$Element[]) prepare_for_register_allocation (after) /// CHECK: <> ParameterValue @@ -55,7 +55,7 @@ public static void main(String args[]) { /// CHECK-DAG: <> NullConstant /// CHECK-DAG: <> LoadClass /// CHECK-DAG: <> ArrayLength [<>] - /// CHECK-DAG: <> ArraySet [<>,<>,<>] needs_type_check:false + /// CHECK-DAG: <> ArraySet [<>,<>,<>] needs_type_check:false can_trigger_gc:false static void testArraySetCheckCastNull(Element[] elements) { Object object = null;