Quick: Fix LVN/GVN handling of acquire operations.
Acquire operations, i.e. MONITOR_ENTER and volatile GETs,
change the thread's view of the memory, so subsequent loads
must get new value names in LVN/GVN. Release operations do
not affect this thread's view of the memory, they the only
push the modifications for other threads to see.
Bug: 17689750
Change-Id: I9442d89b1d2c5252b99b02851b71bb85f871d734
diff --git a/compiler/dex/local_value_numbering.cc b/compiler/dex/local_value_numbering.cc
index 4279955..eb0806b 100644
--- a/compiler/dex/local_value_numbering.cc
+++ b/compiler/dex/local_value_numbering.cc
@@ -1169,8 +1169,9 @@
const MirFieldInfo& field_info = gvn_->GetMirGraph()->GetIFieldLoweringInfo(mir);
uint16_t res;
if (!field_info.IsResolved() || field_info.IsVolatile()) {
- // Volatile fields always get a new memory version; field id is irrelevant.
// Unresolved fields may be volatile, so handle them as such to be safe.
+ HandleInvokeOrClInitOrAcquireOp(mir); // Volatile GETs have acquire semantics.
+ // Volatile fields always get a new memory version; field id is irrelevant.
// Use result s_reg - will be unique.
res = gvn_->LookupValue(kNoValue, mir->ssa_rep->defs[0], kNoValue, kNoValue);
} else {
@@ -1269,14 +1270,16 @@
uint16_t LocalValueNumbering::HandleSGet(MIR* mir, uint16_t opcode) {
const MirSFieldLoweringInfo& field_info = gvn_->GetMirGraph()->GetSFieldLoweringInfo(mir);
- if (!field_info.IsInitialized() && (mir->optimization_flags & MIR_IGNORE_CLINIT_CHECK) == 0) {
- // Class initialization can call arbitrary functions, we need to wipe aliasing values.
- HandleInvokeOrClInit(mir);
+ if (!field_info.IsResolved() || field_info.IsVolatile() ||
+ (!field_info.IsInitialized() && (mir->optimization_flags & MIR_IGNORE_CLINIT_CHECK) == 0)) {
+ // Volatile SGETs (and unresolved fields are potentially volatile) have acquire semantics
+ // and class initialization can call arbitrary functions, we need to wipe aliasing values.
+ HandleInvokeOrClInitOrAcquireOp(mir);
}
uint16_t res;
if (!field_info.IsResolved() || field_info.IsVolatile()) {
- // Volatile fields always get a new memory version; field id is irrelevant.
// Unresolved fields may be volatile, so handle them as such to be safe.
+ // Volatile fields always get a new memory version; field id is irrelevant.
// Use result s_reg - will be unique.
res = gvn_->LookupValue(kNoValue, mir->ssa_rep->defs[0], kNoValue, kNoValue);
} else {
@@ -1306,7 +1309,7 @@
const MirSFieldLoweringInfo& field_info = gvn_->GetMirGraph()->GetSFieldLoweringInfo(mir);
if (!field_info.IsInitialized() && (mir->optimization_flags & MIR_IGNORE_CLINIT_CHECK) == 0) {
// Class initialization can call arbitrary functions, we need to wipe aliasing values.
- HandleInvokeOrClInit(mir);
+ HandleInvokeOrClInitOrAcquireOp(mir);
}
uint16_t type = opcode - Instruction::SPUT;
if (!field_info.IsResolved()) {
@@ -1351,7 +1354,7 @@
}
}
-void LocalValueNumbering::HandleInvokeOrClInit(MIR* mir) {
+void LocalValueNumbering::HandleInvokeOrClInitOrAcquireOp(MIR* mir) {
// Use mir->offset as modifier; without elaborate inlining, it will be unique.
global_memory_version_ =
gvn_->LookupValue(kInvokeMemoryVersionBumpOp, 0u, 0u, mir->offset);
@@ -1404,9 +1407,7 @@
case Instruction::MONITOR_ENTER:
HandleNullCheck(mir, GetOperandValue(mir->ssa_rep->uses[0]));
- // NOTE: Keeping all aliasing values intact. Programs that rely on loads/stores of the
- // same non-volatile locations outside and inside a synchronized block being different
- // contain races that we cannot fix.
+ HandleInvokeOrClInitOrAcquireOp(mir); // Acquire operation.
break;
case Instruction::MONITOR_EXIT:
@@ -1468,7 +1469,7 @@
uint16_t reg = GetOperandValue(mir->ssa_rep->uses[i]);
non_aliasing_refs_.erase(reg);
}
- HandleInvokeOrClInit(mir);
+ HandleInvokeOrClInitOrAcquireOp(mir);
}
break;
diff --git a/compiler/dex/local_value_numbering.h b/compiler/dex/local_value_numbering.h
index e11c6e5..c60da32 100644
--- a/compiler/dex/local_value_numbering.h
+++ b/compiler/dex/local_value_numbering.h
@@ -308,7 +308,7 @@
uint16_t HandleSGet(MIR* mir, uint16_t opcode);
void HandleSPut(MIR* mir, uint16_t opcode);
void RemoveSFieldsForType(uint16_t type);
- void HandleInvokeOrClInit(MIR* mir);
+ void HandleInvokeOrClInitOrAcquireOp(MIR* mir);
bool SameMemoryVersion(const LocalValueNumbering& other) const;
diff --git a/compiler/dex/local_value_numbering_test.cc b/compiler/dex/local_value_numbering_test.cc
index e53c640..067bea2 100644
--- a/compiler/dex/local_value_numbering_test.cc
+++ b/compiler/dex/local_value_numbering_test.cc
@@ -338,16 +338,19 @@
DEF_IGET(Instruction::IGET, 1u, 0u, 0u), // Non-volatile.
DEF_IGET(Instruction::IGET, 2u, 10u, 1u), // Volatile.
DEF_IGET(Instruction::IGET, 3u, 2u, 1u), // Non-volatile.
+ DEF_IGET(Instruction::IGET, 4u, 0u, 0u), // Non-volatile.
};
PrepareIFields(ifields);
PrepareMIRs(mirs);
PerformLVN();
- ASSERT_EQ(value_names_.size(), 4u);
+ ASSERT_EQ(value_names_.size(), 5u);
EXPECT_NE(value_names_[0], value_names_[2]); // Volatile has always different value name.
EXPECT_NE(value_names_[1], value_names_[3]); // Used different base because of volatile.
+ EXPECT_NE(value_names_[1], value_names_[4]); // Not guaranteed to be the same after "acquire".
+
for (size_t i = 0; i != arraysize(mirs); ++i) {
- EXPECT_EQ((i == 2u) ? MIR_IGNORE_NULL_CHECK : 0,
+ EXPECT_EQ((i == 2u || i == 4u) ? MIR_IGNORE_NULL_CHECK : 0,
mirs_[i].optimization_flags) << i;
}
}
@@ -363,7 +366,7 @@
DEF_IGET(Instruction::IGET, 1u, 20u, 0u), // Resolved field #1, unique object.
DEF_IGET(Instruction::IGET, 2u, 21u, 0u), // Resolved field #1.
DEF_IGET_WIDE(Instruction::IGET_WIDE, 3u, 21u, 1u), // Resolved field #2.
- DEF_IGET(Instruction::IGET, 4u, 22u, 2u), // IGET doesn't clobber anything.
+ DEF_IGET(Instruction::IGET, 4u, 22u, 2u), // Unresolved IGET can be "acquire".
DEF_IGET(Instruction::IGET, 5u, 20u, 0u), // Resolved field #1, unique object.
DEF_IGET(Instruction::IGET, 6u, 21u, 0u), // Resolved field #1.
DEF_IGET_WIDE(Instruction::IGET_WIDE, 7u, 21u, 1u), // Resolved field #2.
@@ -381,14 +384,15 @@
PrepareMIRs(mirs);
PerformLVN();
ASSERT_EQ(value_names_.size(), 16u);
- EXPECT_EQ(value_names_[1], value_names_[5]);
- EXPECT_EQ(value_names_[2], value_names_[6]);
- EXPECT_EQ(value_names_[3], value_names_[7]);
- EXPECT_EQ(value_names_[1], value_names_[9]);
- EXPECT_NE(value_names_[2], value_names_[10]); // This aliased with unresolved IPUT.
- EXPECT_EQ(value_names_[3], value_names_[11]);
- EXPECT_EQ(value_names_[12], value_names_[15]);
- EXPECT_NE(value_names_[1], value_names_[14]); // This aliased with unresolved IPUT.
+ // Unresolved field is potentially volatile, so we need to adhere to the volatile semantics.
+ EXPECT_EQ(value_names_[1], value_names_[5]); // Unique object.
+ EXPECT_NE(value_names_[2], value_names_[6]); // Not guaranteed to be the same after "acquire".
+ EXPECT_NE(value_names_[3], value_names_[7]); // Not guaranteed to be the same after "acquire".
+ EXPECT_EQ(value_names_[1], value_names_[9]); // Unique object.
+ EXPECT_NE(value_names_[6], value_names_[10]); // This aliased with unresolved IPUT.
+ EXPECT_EQ(value_names_[7], value_names_[11]); // Still the same after "release".
+ EXPECT_EQ(value_names_[12], value_names_[15]); // Still the same after "release".
+ EXPECT_NE(value_names_[1], value_names_[14]); // This aliased with unresolved IPUT.
EXPECT_EQ(mirs_[0].optimization_flags, 0u);
EXPECT_EQ(mirs_[1].optimization_flags, MIR_IGNORE_NULL_CHECK);
EXPECT_EQ(mirs_[2].optimization_flags, 0u);
@@ -409,7 +413,7 @@
static const MIRDef mirs[] = {
DEF_SGET(Instruction::SGET, 0u, 0u), // Resolved field #1.
DEF_SGET_WIDE(Instruction::SGET_WIDE, 1u, 1u), // Resolved field #2.
- DEF_SGET(Instruction::SGET, 2u, 2u), // SGET doesn't clobber anything.
+ DEF_SGET(Instruction::SGET, 2u, 2u), // Unresolved SGET can be "acquire".
DEF_SGET(Instruction::SGET, 3u, 0u), // Resolved field #1.
DEF_SGET_WIDE(Instruction::SGET_WIDE, 4u, 1u), // Resolved field #2.
DEF_SPUT(Instruction::SPUT, 5u, 2u), // SPUT clobbers field #1 (#2 is wide).
@@ -421,10 +425,11 @@
PrepareMIRs(mirs);
PerformLVN();
ASSERT_EQ(value_names_.size(), 8u);
- EXPECT_EQ(value_names_[0], value_names_[3]);
- EXPECT_EQ(value_names_[1], value_names_[4]);
- EXPECT_NE(value_names_[0], value_names_[6]); // This aliased with unresolved IPUT.
- EXPECT_EQ(value_names_[1], value_names_[7]);
+ // Unresolved field is potentially volatile, so we need to adhere to the volatile semantics.
+ EXPECT_NE(value_names_[0], value_names_[3]); // Not guaranteed to be the same after "acquire".
+ EXPECT_NE(value_names_[1], value_names_[4]); // Not guaranteed to be the same after "acquire".
+ EXPECT_NE(value_names_[3], value_names_[6]); // This aliased with unresolved IPUT.
+ EXPECT_EQ(value_names_[4], value_names_[7]); // Still the same after "release".
for (size_t i = 0u; i != mir_count_; ++i) {
EXPECT_EQ(0, mirs_[i].optimization_flags) << i;
}