Skip to content

Commit

Permalink
Update CanTriggerGC flag for ArraySet
Browse files Browse the repository at this point in the history 
ArraySet instructions can trigger a GC only when perforing a type
check. When clearing the needs type check flag, we should also
clear the CanTriggerGC flag from the instruction's side effects.
Note that once we clear the needs type check flag, we never set
it again.

Add check in graph checker that the needs type check and the can
trigger GC flag are consistent.

Add can_trigger_gc property to graph visualizer that reflects
whether the ArraySet instruction can trigger a GC.

Test: art/test/testrunner/testrunner.py --host --64 --optimizing -b
Change-Id: I4c74f902aabf2339bd292e9b24737f55d2737440
  • Loading branch information
Santiago Aboy Solanes committed Dec 5, 2022
1 parent 6d2b6ba commit 13c3ce1
Show file tree
Hide file tree
Showing 9 changed files with 89 additions and 25 deletions.
15 changes: 15 additions & 0 deletions compiler/optimizing/graph_checker.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 1 addition & 0 deletions compiler/optimizing/graph_checker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions compiler/optimizing/graph_visualizer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
8 changes: 4 additions & 4 deletions compiler/optimizing/instruction_simplifier.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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();
Expand Down
4 changes: 3 additions & 1 deletion compiler/optimizing/nodes.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6597,8 +6597,10 @@ class HArraySet final : public HExpression<3> {
return false;
}

void ClearNeedsTypeCheck() {
void ClearTypeCheck() {
SetPackedFlag<kFlagNeedsTypeCheck>(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() {
Expand Down
2 changes: 1 addition & 1 deletion compiler/optimizing/prepare_for_register_allocation.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}
Expand Down
62 changes: 50 additions & 12 deletions test/521-checker-array-set-null/src/Main.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
13 changes: 9 additions & 4 deletions test/527-checker-array-access-split/src/Main.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -593,12 +593,16 @@ public final static void checkGVNForFatalChecks(int begin, int end, char[] buf1,
/// CHECK: ArrayGet [<<IntAddr1>>,{{i\d+}}]
/// CHECK: <<IntAddr2:i\d+>> IntermediateAddress [<<Array>>,<<DataOffset>>]
/// CHECK: ArrayGet [<<IntAddr2>>,{{i\d+}}]
/// CHECK: ArraySet [<<Array>>,{{i\d+}},{{l\d+}}]
/// CHECK: ArraySet [<<Array>>,{{i\d+}},{{l\d+}}] needs_type_check:false can_trigger_gc:false
/// CHECK: <<IntAddr3:i\d+>> IntermediateAddress [<<Array>>,<<DataOffset>>]
/// CHECK: ArrayGet [<<IntAddr3>>,{{i\d+}}]
/// CHECK: ArraySet [<<Array>>,{{i\d+}},{{l\d+}}]
/// CHECK: ArraySet [<<Array>>,{{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)
Expand All @@ -608,12 +612,13 @@ public final static void checkGVNForFatalChecks(int begin, int end, char[] buf1,
/// CHECK: <<IntAddr1:i\d+>> IntermediateAddress [<<Array>>,<<DataOffset>>]
/// CHECK: ArrayGet [<<IntAddr1>>,{{i\d+}}]
/// CHECK: ArrayGet [<<IntAddr1>>,{{i\d+}}]
/// CHECK: ArraySet [<<Array>>,{{i\d+}},{{l\d+}}]
/// CHECK: <<IntAddr3:i\d+>> IntermediateAddress [<<Array>>,<<DataOffset>>]
/// CHECK: ArrayGet [<<IntAddr3>>,{{i\d+}}]
/// CHECK: ArraySet [<<Array>>,{{i\d+}},{{l\d+}}] needs_type_check:false can_trigger_gc:false
/// CHECK: ArrayGet [<<IntAddr1>>,{{i\d+}}]
/// CHECK: ArraySet [<<Array>>,{{i\d+}},{{l\d+}}]
/// CHECK: ArraySet [<<Array>>,{{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);
Expand Down
6 changes: 3 additions & 3 deletions test/590-checker-arr-set-null-regression/src/Main.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public static void main(String args[]) {
/// CHECK-DAG: <<CheckedArray:l\d+>> NullCheck [<<Array>>]
/// CHECK-DAG: <<Length:i\d+>> ArrayLength [<<CheckedArray>>]
/// CHECK-DAG: <<CheckedIndex:i\d+>> BoundsCheck [<<Index>>,<<Length>>]
/// CHECK-DAG: <<ArraySet:v\d+>> ArraySet [<<CheckedArray>>,<<CheckedIndex>>,<<CheckedValue>>] needs_type_check:true
/// CHECK-DAG: <<ArraySet:v\d+>> ArraySet [<<CheckedArray>>,<<CheckedIndex>>,<<CheckedValue>>] needs_type_check:true can_trigger_gc:true

/// CHECK-START: void Main.testArraySetCheckCastNull(Main$Element[]) instruction_simplifier (after)
/// CHECK-NOT: CheckCast
Expand All @@ -47,15 +47,15 @@ public static void main(String args[]) {
/// CHECK-DAG: <<CheckedArray:l\d+>> NullCheck [<<Array>>]
/// CHECK-DAG: <<Length:i\d+>> ArrayLength [<<CheckedArray>>]
/// CHECK-DAG: <<CheckedIndex:i\d+>> BoundsCheck [<<Index>>,<<Length>>]
/// CHECK-DAG: <<ArraySet:v\d+>> ArraySet [<<CheckedArray>>,<<CheckedIndex>>,<<CheckedValue>>] needs_type_check:true
/// CHECK-DAG: <<ArraySet:v\d+>> ArraySet [<<CheckedArray>>,<<CheckedIndex>>,<<CheckedValue>>] needs_type_check:true can_trigger_gc:true

/// CHECK-START: void Main.testArraySetCheckCastNull(Main$Element[]) prepare_for_register_allocation (after)
/// CHECK: <<Array:l\d+>> ParameterValue
/// CHECK-DAG: <<Index:i\d+>> IntConstant 42
/// CHECK-DAG: <<Null:l\d+>> NullConstant
/// CHECK-DAG: <<Class:l\d+>> LoadClass
/// CHECK-DAG: <<Length:i\d+>> ArrayLength [<<Array>>]
/// CHECK-DAG: <<ArraySet:v\d+>> ArraySet [<<Array>>,<<Index>>,<<Null>>] needs_type_check:false
/// CHECK-DAG: <<ArraySet:v\d+>> ArraySet [<<Array>>,<<Index>>,<<Null>>] needs_type_check:false can_trigger_gc:false

static void testArraySetCheckCastNull(Element[] elements) {
Object object = null;
Expand Down

0 comments on commit 13c3ce1

Please sign in to comment.