Real fix for 064
The recent ssa cleanup CL surfaced a somewhat subtle bug in
live register tracking. The code generation register utilities
attempt to remember and reuse live Dalvik register values for future
use. This remembering takes place in the storeValueXX() code.
For this to work, though, storeValue may only be called once during
the compilation of any single Dalvik instruction.
However, the code generation routine for CONST_CLASS included a
somewhat complicated slow path with iternal branches and two
generated "storeValue" locations. This resulted in downstream
code expecting to find a live value in the wrong place.
This fix is to note this special case and do a "clobber" on the ssa name.
This CL also includes some sanity checking code that can detect
multiple calls to storeValue during one intruction compilation to
try to catch this situation in the future.
Change-Id: I66a279140accd80cda83f66efe570c9702fb351b
diff --git a/src/compiler/CompilerIR.h b/src/compiler/CompilerIR.h
index bd4c156..5750913 100644
--- a/src/compiler/CompilerIR.h
+++ b/src/compiler/CompilerIR.h
@@ -414,6 +414,14 @@
struct ArenaMemBlock* currentArena;
int numArenaBlocks;
struct Memstats* mstats;
+#ifndef NDEBUG
+ /*
+ * Sanity checking for the register temp tracking. The same ssa
+ * name should never be associated with one temp register per
+ * instruction compilation.
+ */
+ int liveSReg;
+#endif
};
enum OpSize {
diff --git a/src/compiler/SSATransformation.cc b/src/compiler/SSATransformation.cc
index bd5f83b..c5a6b8f 100644
--- a/src/compiler/SSATransformation.cc
+++ b/src/compiler/SSATransformation.cc
@@ -26,14 +26,22 @@
if (block->visited || block->hidden) return;
block->visited = true;
+ // Can this block be reached only via previous block fallthrough?
+ if ((block->blockType == kDalvikByteCode) &&
+ (block->predecessors->numUsed == 1)) {
+ DCHECK_GE(cUnit->dfsOrder.numUsed, 1U);
+ int prevIdx = cUnit->dfsOrder.numUsed - 1;
+ int prevId = cUnit->dfsOrder.elemList[prevIdx];
+ BasicBlock* predBB = (BasicBlock*)block->predecessors->elemList[0];
+ if (predBB->id == prevId) {
+ block->fallThroughTarget = true;
+ }
+ }
+
/* Enqueue the preOrder block id */
oatInsertGrowableList(cUnit, &cUnit->dfsOrder, block->id);
if (block->fallThrough) {
-#if 0
- // Temporary bug workaround
- block->fallThrough->fallThroughTarget = true;
-#endif
recordDFSOrders(cUnit, block->fallThrough);
}
if (block->taken) recordDFSOrders(cUnit, block->taken);
diff --git a/src/compiler/codegen/CodegenFactory.cc b/src/compiler/codegen/CodegenFactory.cc
index 5444816..eebaaf3 100644
--- a/src/compiler/codegen/CodegenFactory.cc
+++ b/src/compiler/codegen/CodegenFactory.cc
@@ -134,6 +134,16 @@
void storeValue(CompilationUnit* cUnit, RegLocation rlDest, RegLocation rlSrc)
{
+#ifndef NDEBUG
+ /*
+ * Sanity checking - should never try to store to the same
+ * ssa name during the compilation of a single instruction
+ * without an intervening oatClobberSReg().
+ */
+ DCHECK((cUnit->liveSReg == INVALID_SREG) ||
+ (rlDest.sRegLow != cUnit->liveSReg));
+ cUnit->liveSReg = rlDest.sRegLow;
+#endif
LIR* defStart;
LIR* defEnd;
DCHECK(!rlDest.wide);
@@ -195,6 +205,16 @@
void storeValueWide(CompilationUnit* cUnit, RegLocation rlDest,
RegLocation rlSrc)
{
+#ifndef NDEBUG
+ /*
+ * Sanity checking - should never try to store to the same
+ * ssa name during the compilation of a single instruction
+ * without an intervening oatClobberSReg().
+ */
+ DCHECK((cUnit->liveSReg == INVALID_SREG) ||
+ (rlDest.sRegLow != cUnit->liveSReg));
+ cUnit->liveSReg = rlDest.sRegLow;
+#endif
LIR* defStart;
LIR* defEnd;
DCHECK_EQ(FPREG(rlSrc.lowReg), FPREG(rlSrc.highReg));
diff --git a/src/compiler/codegen/GenCommon.cc b/src/compiler/codegen/GenCommon.cc
index 9b1654f..72a596a 100644
--- a/src/compiler/codegen/GenCommon.cc
+++ b/src/compiler/codegen/GenCommon.cc
@@ -870,6 +870,11 @@
NULL);
// Resolved, store and hop over following code
storeValue(cUnit, rlDest, rlResult);
+ /*
+ * Because we have stores of the target value on two paths,
+ * clobber temp tracking for the destination using the ssa name
+ */
+ oatClobberSReg(cUnit, rlDest.sRegLow);
LIR* branch2 = opUnconditionalBranch(cUnit,0);
// TUNING: move slow path to end & remove unconditional branch
LIR* target1 = newLIR0(cUnit, kPseudoTargetLabel);
@@ -881,6 +886,11 @@
callRuntimeHelper(cUnit, rTgt);
RegLocation rlResult = oatGetReturn(cUnit);
storeValue(cUnit, rlDest, rlResult);
+ /*
+ * Because we have stores of the target value on two paths,
+ * clobber temp tracking for the destination using the ssa name
+ */
+ oatClobberSReg(cUnit, rlDest.sRegLow);
// Rejoin code paths
LIR* target2 = newLIR0(cUnit, kPseudoTargetLabel);
branch1->target = (LIR*)target1;
diff --git a/src/compiler/codegen/MethodCodegenDriver.cc b/src/compiler/codegen/MethodCodegenDriver.cc
index 5baabf2..6aa6366 100644
--- a/src/compiler/codegen/MethodCodegenDriver.cc
+++ b/src/compiler/codegen/MethodCodegenDriver.cc
@@ -795,6 +795,11 @@
oatResetDefTracking(cUnit);
}
+#ifndef NDEBUG
+ /* Reset temp tracking sanity check */
+ cUnit->liveSReg = INVALID_SREG;
+#endif
+
if ((int)mir->dalvikInsn.opcode >= (int)kMirOpFirst) {
handleExtendedMethodMIR(cUnit, mir);
continue;
diff --git a/src/compiler/codegen/RallocUtil.cc b/src/compiler/codegen/RallocUtil.cc
index 8f5d1bb..d5f6720 100644
--- a/src/compiler/codegen/RallocUtil.cc
+++ b/src/compiler/codegen/RallocUtil.cc
@@ -141,6 +141,12 @@
/* Clobber any temp associated with an sReg. Could be in either class */
extern void oatClobberSReg(CompilationUnit* cUnit, int sReg)
{
+#ifndef NDEBUG
+ /* Reset live temp tracking sanity checker */
+ if (sReg == cUnit->liveSReg) {
+ cUnit->liveSReg = INVALID_SREG;
+ }
+#endif
clobberSRegBody(cUnit->regPool->coreRegs, cUnit->regPool->numCoreRegs,
sReg);
clobberSRegBody(cUnit->regPool->FPRegs, cUnit->regPool->numFPRegs,