ART: Resolve ambiguous ArraySets
Just like aget(-wide), the value operand of aput(-wide) bytecode
instructions can be both int/long and float/double. This patch builds
on the previous mechanism for resolving type of ArrayGets to type the
values of ArraySets based on the reference type of the array.
Bug: 22538329
Change-Id: Ic86abbb58de146692de04476b555010b6fcdd8b6
diff --git a/compiler/optimizing/induction_var_analysis_test.cc b/compiler/optimizing/induction_var_analysis_test.cc
index 776c115..29a1845 100644
--- a/compiler/optimizing/induction_var_analysis_test.cc
+++ b/compiler/optimizing/induction_var_analysis_test.cc
@@ -85,6 +85,7 @@
constant0_ = graph_->GetIntConstant(0);
constant1_ = graph_->GetIntConstant(1);
constant100_ = graph_->GetIntConstant(100);
+ float_constant0_ = graph_->GetFloatConstant(0.0f);
induc_ = new (&allocator_) HLocal(n);
entry_->AddInstruction(induc_);
entry_->AddInstruction(new (&allocator_) HStoreLocal(induc_, constant0_));
@@ -156,8 +157,10 @@
HInstruction* InsertArrayStore(HLocal* subscript, int d) {
HInstruction* load = InsertInstruction(
new (&allocator_) HLoadLocal(subscript, Primitive::kPrimInt), d);
+ // ArraySet is given a float value in order to avoid SsaBuilder typing
+ // it from the array's non-existent reference type info.
return InsertInstruction(new (&allocator_) HArraySet(
- parameter_, load, constant0_, Primitive::kPrimInt, 0), d);
+ parameter_, load, float_constant0_, Primitive::kPrimFloat, 0), d);
}
// Returns induction information of instruction in loop at depth d.
@@ -187,6 +190,7 @@
HInstruction* constant0_;
HInstruction* constant1_;
HInstruction* constant100_;
+ HInstruction* float_constant0_;
HLocal* induc_; // "vreg_n", the "k"
HLocal* tmp_; // "vreg_n+1"
HLocal* dum_; // "vreg_n+2"
diff --git a/compiler/optimizing/licm_test.cc b/compiler/optimizing/licm_test.cc
index aa60fd6..2b63ec8 100644
--- a/compiler/optimizing/licm_test.cc
+++ b/compiler/optimizing/licm_test.cc
@@ -65,7 +65,8 @@
// Provide boiler-plate instructions.
parameter_ = new (&allocator_) HParameterValue(graph_->GetDexFile(), 0, 0, Primitive::kPrimNot);
entry_->AddInstruction(parameter_);
- constant_ = graph_->GetIntConstant(42);
+ int_constant_ = graph_->GetIntConstant(42);
+ float_constant_ = graph_->GetFloatConstant(42.0f);
loop_preheader_->AddInstruction(new (&allocator_) HGoto());
loop_header_->AddInstruction(new (&allocator_) HIf(parameter_));
loop_body_->AddInstruction(new (&allocator_) HGoto());
@@ -95,7 +96,8 @@
HBasicBlock* exit_;
HInstruction* parameter_; // "this"
- HInstruction* constant_;
+ HInstruction* int_constant_;
+ HInstruction* float_constant_;
};
//
@@ -118,7 +120,7 @@
0);
loop_body_->InsertInstructionBefore(get_field, loop_body_->GetLastInstruction());
HInstruction* set_field = new (&allocator_) HInstanceFieldSet(
- parameter_, constant_, Primitive::kPrimInt, MemberOffset(20),
+ parameter_, int_constant_, Primitive::kPrimInt, MemberOffset(20),
false, kUnknownFieldIndex, kUnknownClassDefIndex, graph_->GetDexFile(), dex_cache, 0);
loop_body_->InsertInstructionBefore(set_field, loop_body_->GetLastInstruction());
@@ -167,11 +169,13 @@
BuildLoop();
// Populate the loop with instructions: set/get array with different types.
+ // ArrayGet is typed as kPrimByte and ArraySet given a float value in order to
+ // avoid SsaBuilder's typing of ambiguous array operations from reference type info.
HInstruction* get_array = new (&allocator_) HArrayGet(
- parameter_, constant_, Primitive::kPrimByte, 0);
+ parameter_, int_constant_, Primitive::kPrimByte, 0);
loop_body_->InsertInstructionBefore(get_array, loop_body_->GetLastInstruction());
HInstruction* set_array = new (&allocator_) HArraySet(
- parameter_, constant_, constant_, Primitive::kPrimShort, 0);
+ parameter_, int_constant_, float_constant_, Primitive::kPrimShort, 0);
loop_body_->InsertInstructionBefore(set_array, loop_body_->GetLastInstruction());
EXPECT_EQ(get_array->GetBlock(), loop_body_);
@@ -185,11 +189,13 @@
BuildLoop();
// Populate the loop with instructions: set/get array with same types.
+ // ArrayGet is typed as kPrimByte and ArraySet given a float value in order to
+ // avoid SsaBuilder's typing of ambiguous array operations from reference type info.
HInstruction* get_array = new (&allocator_) HArrayGet(
- parameter_, constant_, Primitive::kPrimByte, 0);
+ parameter_, int_constant_, Primitive::kPrimByte, 0);
loop_body_->InsertInstructionBefore(get_array, loop_body_->GetLastInstruction());
HInstruction* set_array = new (&allocator_) HArraySet(
- parameter_, get_array, constant_, Primitive::kPrimByte, 0);
+ parameter_, get_array, float_constant_, Primitive::kPrimByte, 0);
loop_body_->InsertInstructionBefore(set_array, loop_body_->GetLastInstruction());
EXPECT_EQ(get_array->GetBlock(), loop_body_);
diff --git a/compiler/optimizing/load_store_elimination.cc b/compiler/optimizing/load_store_elimination.cc
index 727f2bb..2b313f6 100644
--- a/compiler/optimizing/load_store_elimination.cc
+++ b/compiler/optimizing/load_store_elimination.cc
@@ -678,16 +678,6 @@
}
}
- static bool IsIntFloatAlias(Primitive::Type type1, Primitive::Type type2) {
- return (type1 == Primitive::kPrimFloat && type2 == Primitive::kPrimInt) ||
- (type2 == Primitive::kPrimFloat && type1 == Primitive::kPrimInt);
- }
-
- static bool IsLongDoubleAlias(Primitive::Type type1, Primitive::Type type2) {
- return (type1 == Primitive::kPrimDouble && type2 == Primitive::kPrimLong) ||
- (type2 == Primitive::kPrimDouble && type1 == Primitive::kPrimLong);
- }
-
void VisitGetLocation(HInstruction* instruction,
HInstruction* ref,
size_t offset,
@@ -716,22 +706,14 @@
// Get the real heap value of the store.
heap_value = store->InputAt(1);
}
- if ((heap_value != kUnknownHeapValue) &&
- // Keep the load due to possible I/F, J/D array aliasing.
- // See b/22538329 for details.
- !IsIntFloatAlias(heap_value->GetType(), instruction->GetType()) &&
- !IsLongDoubleAlias(heap_value->GetType(), instruction->GetType())) {
+ if (heap_value == kUnknownHeapValue) {
+ // Load isn't eliminated. Put the load as the value into the HeapLocation.
+ // This acts like GVN but with better aliasing analysis.
+ heap_values[idx] = instruction;
+ } else {
removed_loads_.push_back(instruction);
substitute_instructions_for_loads_.push_back(heap_value);
TryRemovingNullCheck(instruction);
- return;
- }
-
- // Load isn't eliminated.
- if (heap_value == kUnknownHeapValue) {
- // Put the load as the value into the HeapLocation.
- // This acts like GVN but with better aliasing analysis.
- heap_values[idx] = instruction;
}
}
diff --git a/compiler/optimizing/nodes.h b/compiler/optimizing/nodes.h
index dc54bda..1a7cbde 100644
--- a/compiler/optimizing/nodes.h
+++ b/compiler/optimizing/nodes.h
@@ -101,7 +101,7 @@
enum BuildSsaResult {
kBuildSsaFailNonNaturalLoop,
kBuildSsaFailThrowCatchLoop,
- kBuildSsaFailAmbiguousArrayGet,
+ kBuildSsaFailAmbiguousArrayOp,
kBuildSsaSuccess,
};
diff --git a/compiler/optimizing/optimizing_compiler.cc b/compiler/optimizing/optimizing_compiler.cc
index 3f9e151..3eb7274 100644
--- a/compiler/optimizing/optimizing_compiler.cc
+++ b/compiler/optimizing/optimizing_compiler.cc
@@ -786,8 +786,8 @@
case kBuildSsaFailThrowCatchLoop:
MaybeRecordStat(MethodCompilationStat::kNotCompiledThrowCatchLoop);
break;
- case kBuildSsaFailAmbiguousArrayGet:
- MaybeRecordStat(MethodCompilationStat::kNotCompiledAmbiguousArrayGet);
+ case kBuildSsaFailAmbiguousArrayOp:
+ MaybeRecordStat(MethodCompilationStat::kNotCompiledAmbiguousArrayOp);
break;
case kBuildSsaSuccess:
UNREACHABLE();
diff --git a/compiler/optimizing/optimizing_compiler_stats.h b/compiler/optimizing/optimizing_compiler_stats.h
index 4713514..bca1632 100644
--- a/compiler/optimizing/optimizing_compiler_stats.h
+++ b/compiler/optimizing/optimizing_compiler_stats.h
@@ -40,7 +40,7 @@
kNotCompiledBranchOutsideMethodCode,
kNotCompiledNonNaturalLoop,
kNotCompiledThrowCatchLoop,
- kNotCompiledAmbiguousArrayGet,
+ kNotCompiledAmbiguousArrayOp,
kNotCompiledHugeMethod,
kNotCompiledLargeMethodNoBranches,
kNotCompiledMalformedOpcode,
@@ -108,7 +108,7 @@
case kNotCompiledBranchOutsideMethodCode: name = "NotCompiledBranchOutsideMethodCode"; break;
case kNotCompiledNonNaturalLoop : name = "NotCompiledNonNaturalLoop"; break;
case kNotCompiledThrowCatchLoop : name = "NotCompiledThrowCatchLoop"; break;
- case kNotCompiledAmbiguousArrayGet : name = "NotCompiledAmbiguousArrayGet"; break;
+ case kNotCompiledAmbiguousArrayOp : name = "NotCompiledAmbiguousArrayOp"; break;
case kNotCompiledHugeMethod : name = "NotCompiledHugeMethod"; break;
case kNotCompiledLargeMethodNoBranches : name = "NotCompiledLargeMethodNoBranches"; break;
case kNotCompiledMalformedOpcode : name = "NotCompiledMalformedOpcode"; break;
diff --git a/compiler/optimizing/ssa_builder.cc b/compiler/optimizing/ssa_builder.cc
index 9ea352d..f6bab8e 100644
--- a/compiler/optimizing/ssa_builder.cc
+++ b/compiler/optimizing/ssa_builder.cc
@@ -317,27 +317,15 @@
return equivalent;
}
-// Returns true if the array input of `aget` is either of type int[] or long[].
-// Should only be called on ArrayGets with ambiguous type (int/float, long/double)
-// on arrays which were typed to an array class by RTP.
-static bool IsArrayGetOnIntegralArray(HArrayGet* aget) SHARED_REQUIRES(Locks::mutator_lock_) {
- ReferenceTypeInfo array_type = aget->GetArray()->GetReferenceTypeInfo();
+static Primitive::Type GetPrimitiveArrayComponentType(HInstruction* array)
+ SHARED_REQUIRES(Locks::mutator_lock_) {
+ ReferenceTypeInfo array_type = array->GetReferenceTypeInfo();
DCHECK(array_type.IsPrimitiveArrayClass());
- ReferenceTypeInfo::TypeHandle array_type_handle = array_type.GetTypeHandle();
-
- bool is_integral_type;
- if (Primitive::Is64BitType(aget->GetType())) {
- is_integral_type = array_type_handle->GetComponentType()->IsPrimitiveLong();
- DCHECK(is_integral_type || array_type_handle->GetComponentType()->IsPrimitiveDouble());
- } else {
- is_integral_type = array_type_handle->GetComponentType()->IsPrimitiveInt();
- DCHECK(is_integral_type || array_type_handle->GetComponentType()->IsPrimitiveFloat());
- }
- return is_integral_type;
+ return array_type.GetTypeHandle()->GetComponentType()->GetPrimitiveType();
}
-bool SsaBuilder::FixAmbiguousArrayGets() {
- if (ambiguous_agets_.empty()) {
+bool SsaBuilder::FixAmbiguousArrayOps() {
+ if (ambiguous_agets_.empty() && ambiguous_asets_.empty()) {
return true;
}
@@ -351,13 +339,17 @@
ScopedObjectAccess soa(Thread::Current());
for (HArrayGet* aget_int : ambiguous_agets_) {
- if (!aget_int->GetArray()->GetReferenceTypeInfo().IsPrimitiveArrayClass()) {
+ HInstruction* array = aget_int->GetArray();
+ if (!array->GetReferenceTypeInfo().IsPrimitiveArrayClass()) {
// RTP did not type the input array. Bail.
return false;
}
HArrayGet* aget_float = FindFloatOrDoubleEquivalentOfArrayGet(aget_int);
- if (IsArrayGetOnIntegralArray(aget_int)) {
+ Primitive::Type array_type = GetPrimitiveArrayComponentType(array);
+ DCHECK_EQ(Primitive::Is64BitType(aget_int->GetType()), Primitive::Is64BitType(array_type));
+
+ if (Primitive::IsIntOrLongType(array_type)) {
if (aget_float != nullptr) {
// There is a float/double equivalent. We must replace it and re-run
// primitive type propagation on all dependent instructions.
@@ -366,6 +358,7 @@
AddDependentInstructionsToWorklist(aget_int, &worklist);
}
} else {
+ DCHECK(Primitive::IsFloatingPointType(array_type));
if (aget_float == nullptr) {
// This is a float/double ArrayGet but there were no typed uses which
// would create the typed equivalent. Create it now.
@@ -379,11 +372,47 @@
AddDependentInstructionsToWorklist(aget_float, &worklist);
}
}
- }
- // Set a flag stating that types of ArrayGets have been resolved. This is used
- // by GetFloatOrDoubleEquivalentOfArrayGet to report conflict.
- agets_fixed_ = true;
+ // Set a flag stating that types of ArrayGets have been resolved. Requesting
+ // equivalent of the wrong type with GetFloatOrDoubleEquivalentOfArrayGet
+ // will fail from now on.
+ agets_fixed_ = true;
+
+ for (HArraySet* aset : ambiguous_asets_) {
+ HInstruction* array = aset->GetArray();
+ if (!array->GetReferenceTypeInfo().IsPrimitiveArrayClass()) {
+ // RTP did not type the input array. Bail.
+ return false;
+ }
+
+ HInstruction* value = aset->GetValue();
+ Primitive::Type value_type = value->GetType();
+ Primitive::Type array_type = GetPrimitiveArrayComponentType(array);
+ DCHECK_EQ(Primitive::Is64BitType(value_type), Primitive::Is64BitType(array_type));
+
+ if (Primitive::IsFloatingPointType(array_type)) {
+ if (!Primitive::IsFloatingPointType(value_type)) {
+ DCHECK(Primitive::IsIntegralType(value_type));
+ // Array elements are floating-point but the value has not been replaced
+ // with its floating-point equivalent. The replacement must always
+ // succeed in code validated by the verifier.
+ HInstruction* equivalent = GetFloatOrDoubleEquivalent(value, array_type);
+ DCHECK(equivalent != nullptr);
+ aset->ReplaceInput(equivalent, /* input_index */ 2);
+ if (equivalent->IsPhi()) {
+ // Returned equivalent is a phi which may not have had its inputs
+ // replaced yet. We need to run primitive type propagation on it.
+ worklist.push_back(equivalent->AsPhi());
+ }
+ }
+ } else {
+ // Array elements are integral and the value assigned to it initially
+ // was integral too. Nothing to do.
+ DCHECK(Primitive::IsIntegralType(array_type));
+ DCHECK(Primitive::IsIntegralType(value_type));
+ }
+ }
+ }
if (!worklist.empty()) {
ProcessPrimitiveTypePropagationWorklist(&worklist);
@@ -429,10 +458,11 @@
ReferenceTypePropagation(GetGraph(), handles_).Run();
// 7) Step 1) duplicated ArrayGet instructions with ambiguous type (int/float
- // or long/double). Now that RTP computed the type of the array input, the
- // ambiguity can be resolved and the correct equivalent kept.
- if (!FixAmbiguousArrayGets()) {
- return kBuildSsaFailAmbiguousArrayGet;
+ // or long/double) and marked ArraySets with ambiguous input type. Now that RTP
+ // computed the type of the array input, the ambiguity can be resolved and the
+ // correct equivalents kept.
+ if (!FixAmbiguousArrayOps()) {
+ return kBuildSsaFailAmbiguousArrayOp;
}
// 8) Mark dead phis. This will mark phis which are not used by instructions
@@ -702,7 +732,7 @@
// int/long. Requesting a float/double equivalent should lead to a conflict.
if (kIsDebugBuild) {
ScopedObjectAccess soa(Thread::Current());
- DCHECK(IsArrayGetOnIntegralArray(aget));
+ DCHECK(Primitive::IsIntOrLongType(GetPrimitiveArrayComponentType(aget->GetArray())));
}
return nullptr;
} else {
@@ -847,4 +877,12 @@
VisitInstruction(aget);
}
+void SsaBuilder::VisitArraySet(HArraySet* aset) {
+ Primitive::Type type = aset->GetValue()->GetType();
+ if (Primitive::IsIntOrLongType(type)) {
+ ambiguous_asets_.push_back(aset);
+ }
+ VisitInstruction(aset);
+}
+
} // namespace art
diff --git a/compiler/optimizing/ssa_builder.h b/compiler/optimizing/ssa_builder.h
index ed6f5ca..0fcc3a1 100644
--- a/compiler/optimizing/ssa_builder.h
+++ b/compiler/optimizing/ssa_builder.h
@@ -56,6 +56,7 @@
current_locals_(nullptr),
loop_headers_(graph->GetArena()->Adapter(kArenaAllocSsaBuilder)),
ambiguous_agets_(graph->GetArena()->Adapter(kArenaAllocSsaBuilder)),
+ ambiguous_asets_(graph->GetArena()->Adapter(kArenaAllocSsaBuilder)),
locals_for_(graph->GetBlocks().size(),
ArenaVector<HInstruction*>(graph->GetArena()->Adapter(kArenaAllocSsaBuilder)),
graph->GetArena()->Adapter(kArenaAllocSsaBuilder)) {
@@ -75,6 +76,7 @@
void VisitInstruction(HInstruction* instruction);
void VisitTemporary(HTemporary* instruction);
void VisitArrayGet(HArrayGet* aget);
+ void VisitArraySet(HArraySet* aset);
static constexpr const char* kSsaBuilderPassName = "ssa_builder";
@@ -85,10 +87,10 @@
void EquivalentPhisCleanup();
void RunPrimitiveTypePropagation();
- // Attempts to resolve types of aget and aget-wide instructions from reference
- // type information on the input array. Returns false if the type of the array
- // is unknown.
- bool FixAmbiguousArrayGets();
+ // Attempts to resolve types of aget(-wide) instructions and type values passed
+ // to aput(-wide) instructions from reference type information on the array
+ // input. Returns false if the type of an array is unknown.
+ bool FixAmbiguousArrayOps();
bool TypeInputsOfPhi(HPhi* phi, ArenaVector<HPhi*>* worklist);
bool UpdatePrimitiveType(HPhi* phi, ArenaVector<HPhi*>* worklist);
@@ -115,6 +117,7 @@
ArenaVector<HBasicBlock*> loop_headers_;
ArenaVector<HArrayGet*> ambiguous_agets_;
+ ArenaVector<HArraySet*> ambiguous_asets_;
// HEnvironment for each block.
ArenaVector<ArenaVector<HInstruction*>> locals_for_;
diff --git a/test/552-checker-primitive-typeprop/smali/ArraySet.smali b/test/552-checker-primitive-typeprop/smali/ArraySet.smali
new file mode 100644
index 0000000..57d8606
--- /dev/null
+++ b/test/552-checker-primitive-typeprop/smali/ArraySet.smali
@@ -0,0 +1,51 @@
+# Copyright (C) 2015 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+.class public LArraySet;
+.super Ljava/lang/Object;
+
+# Test ArraySet on int[] and float[] arrays. The input should be typed accordingly.
+# Note that the input is a Phi to make sure primitive type propagation is re-run
+# on the replaced inputs.
+
+## CHECK-START: void ArraySet.ambiguousSet(int[], float[], boolean) ssa_builder (after)
+## CHECK-DAG: <<IntArray:l\d+>> ParameterValue klass:int[]
+## CHECK-DAG: <<IntA:i\d+>> IntConstant 0
+## CHECK-DAG: <<IntB:i\d+>> IntConstant 1073741824
+## CHECK-DAG: <<IntPhi:i\d+>> Phi [<<IntA>>,<<IntB>>] reg:0
+## CHECK-DAG: <<IntNC:l\d+>> NullCheck [<<IntArray>>]
+## CHECK-DAG: ArraySet [<<IntNC>>,{{i\d+}},<<IntPhi>>]
+
+## CHECK-DAG: <<FloatArray:l\d+>> ParameterValue klass:float[]
+## CHECK-DAG: <<FloatA:f\d+>> FloatConstant 0
+## CHECK-DAG: <<FloatB:f\d+>> FloatConstant 2
+## CHECK-DAG: <<FloatPhi:f\d+>> Phi [<<FloatA>>,<<FloatB>>] reg:0
+## CHECK-DAG: <<FloatNC:l\d+>> NullCheck [<<FloatArray>>]
+## CHECK-DAG: ArraySet [<<FloatNC>>,{{i\d+}},<<FloatPhi>>]
+
+.method public static ambiguousSet([I[FZ)V
+ .registers 8
+
+ const v0, 0x0
+ if-eqz p2, :else
+ const v0, 0x40000000
+ :else
+ # v0 = Phi [0.0f, 2.0f]
+
+ const v1, 0x1
+ aput v0, p0, v1
+ aput v0, p1, v1
+
+ return-void
+.end method