Subzero: Fix a bug in register allocator overlap computation.

When the register allocator decides whether to allow the candidate's live range to overlap its preferred variable's live range (and share their register), it needs to consider whether any redefinitions in one variable occur within the live range of the other variable, in which case overlap should not be allowed.

There was a bug in the API for iterating over the defining instructions for a variable, in which the earliest definition might be ignored in some cases.  This came from the fact that the first definition and latter definitions are split apart for translation speed reasons, and a particular API is needed for finding an unambiguous first definition, which is possible when all definitions are within a single block but not so possible when definitions cross block boundaries.  (This only happens for the simple phi lowering.)

Since both semantics are needed, a separate API is added to support both.

For spec2k, the asm output is identical to before, so this changes nothing.  When translating spec2k with "-O2 -phi-edge-split=0", there is a single minor difference in ammp that actually looks legit in both cases.

However, when testing an upcoming CL, -phi-edge-split=0 triggered the bug, causing gcc and crafty to fail with incorrect output.

This CL also fixes some minor issues, and adds dump output of the instruction definition list when available.

BUG= none
R=jpp@chromium.org

Review URL: https://codereview.chromium.org/1381563004 .

diff --git a/src/IceCfg.cpp b/src/IceCfg.cpp
index d89fa49..4c58d50 100644 (file)
--- a/src/IceCfg.cpp
+++ b/src/IceCfg.cpp
@@ -810,6 +810,22 @@ void Cfg::dump(const IceString &Message) {
         Str << getVMetadata()->isMultiBlock(Var);
       else
         Str << "?";
+      Str << " defs=";
+      bool FirstPrint = true;
+      if (VMetadata->getKind() != VMK_Uses) {
+        if (const Inst *FirstDef = VMetadata->getFirstDefinition(Var)) {
+          Str << FirstDef->getNumber();
+          FirstPrint = false;
+        }
+      }
+      if (VMetadata->getKind() == VMK_All) {
+        for (const Inst *Instr : VMetadata->getLatterDefinitions(Var)) {
+          if (!FirstPrint)
+            Str << ",";
+          Str << Instr->getNumber();
+          FirstPrint = false;
+        }
+      }
       Str << " weight=" << Var->getWeight(this) << " ";
       Var->dump(this);
       Str << " LIVE=" << Var->getLiveRange() << "\n";

diff --git a/src/IceCompileServer.cpp b/src/IceCompileServer.cpp
index e2d7e38..7ba5c30 100644 (file)
--- a/src/IceCompileServer.cpp
+++ b/src/IceCompileServer.cpp
@@ -94,8 +94,7 @@ ErrorCodes getReturnValue(const Ice::ClFlagsExtra &Flags, ErrorCodes Val) {
 }
 
 // Reports fatal error message, and then exits with success status 0.
-void reportFatalErrorThenExitSuccess(void * UserData,
-                                     const std::string &Reason,
+void reportFatalErrorThenExitSuccess(void *UserData, const std::string &Reason,
                                      bool GenCrashDag) {
   (void)UserData;
   (void)GenCrashDag;

diff --git a/src/IceOperand.cpp b/src/IceOperand.cpp
index 125c692..66321c2 100644 (file)
--- a/src/IceOperand.cpp
+++ b/src/IceOperand.cpp
@@ -213,15 +213,16 @@ void VariableTracking::markDef(MetadataKind TrackingKind, const Inst *Instr,
   // omit all uses of the variable if markDef() and markUse() both use this
   // optimization.
   assert(Node);
-// Verify that instructions are added in increasing order.
-#ifndef NDEBUG
-  if (TrackingKind == VMK_All) {
-    const Inst *LastInstruction =
-        Definitions.empty() ? FirstOrSingleDefinition : Definitions.back();
-    assert(LastInstruction == nullptr ||
-           Instr->getNumber() >= LastInstruction->getNumber());
+  // Verify that instructions are added in increasing order.
+  if (BuildDefs::asserts()) {
+    if (TrackingKind == VMK_All) {
+      const Inst *LastInstruction =
+          Definitions.empty() ? FirstOrSingleDefinition : Definitions.back();
+      (void)LastInstruction;
+      assert(LastInstruction == nullptr ||
+             Instr->getNumber() >= LastInstruction->getNumber());
+    }
   }
-#endif
   constexpr bool IsImplicit = false;
   markUse(TrackingKind, Instr, Node, IsImplicit);
   if (TrackingKind == VMK_Uses)
@@ -258,7 +259,7 @@ void VariableTracking::markDef(MetadataKind TrackingKind, const Inst *Instr,
   }
 }
 
-const Inst *VariableTracking::getFirstDefinition() const {
+const Inst *VariableTracking::getFirstDefinitionSingleBlock() const {
   switch (MultiDef) {
   case MDS_Unknown:
   case MDS_MultiDefMultiBlock:
@@ -284,6 +285,19 @@ const Inst *VariableTracking::getSingleDefinition() const {
   return nullptr;
 }
 
+const Inst *VariableTracking::getFirstDefinition() const {
+  switch (MultiDef) {
+  case MDS_Unknown:
+    return nullptr;
+  case MDS_MultiDefMultiBlock:
+  case MDS_SingleDef:
+  case MDS_MultiDefSingleBlock:
+    assert(FirstOrSingleDefinition);
+    return FirstOrSingleDefinition;
+  }
+  return nullptr;
+}
+
 void VariablesMetadata::init(MetadataKind TrackingKind) {
   TimerMarker T(TimerStack::TT_vmetadata, Func);
   Kind = TrackingKind;
@@ -303,7 +317,7 @@ void VariablesMetadata::init(MetadataKind TrackingKind) {
 }
 
 void VariablesMetadata::addNode(CfgNode *Node) {
-  if (Func->getNumVariables() >= Metadata.size())
+  if (Func->getNumVariables() > Metadata.size())
     Metadata.resize(Func->getNumVariables());
 
   for (Inst &I : Node->getPhis()) {
@@ -364,12 +378,13 @@ bool VariablesMetadata::isMultiBlock(const Variable *Var) const {
   return Metadata[VarNum].getMultiBlock() != VariableTracking::MBS_SingleBlock;
 }
 
-const Inst *VariablesMetadata::getFirstDefinition(const Variable *Var) const {
+const Inst *
+VariablesMetadata::getFirstDefinitionSingleBlock(const Variable *Var) const {
   assert(Kind != VMK_Uses);
   if (!isTracked(Var))
     return nullptr; // conservative answer
   SizeT VarNum = Var->getIndex();
-  return Metadata[VarNum].getFirstDefinition();
+  return Metadata[VarNum].getFirstDefinitionSingleBlock();
 }
 
 const Inst *VariablesMetadata::getSingleDefinition(const Variable *Var) const {
@@ -380,6 +395,14 @@ const Inst *VariablesMetadata::getSingleDefinition(const Variable *Var) const {
   return Metadata[VarNum].getSingleDefinition();
 }
 
+const Inst *VariablesMetadata::getFirstDefinition(const Variable *Var) const {
+  assert(Kind != VMK_Uses);
+  if (!isTracked(Var))
+    return nullptr; // conservative answer
+  SizeT VarNum = Var->getIndex();
+  return Metadata[VarNum].getFirstDefinition();
+}
+
 const InstDefList &
 VariablesMetadata::getLatterDefinitions(const Variable *Var) const {
   assert(Kind == VMK_All);

diff --git a/src/IceOperand.h b/src/IceOperand.h
index 28c118d..1806f8a 100644 (file)
--- a/src/IceOperand.h
+++ b/src/IceOperand.h
@@ -629,8 +629,9 @@ public:
   VariableTracking(const VariableTracking &) = default;
   MultiDefState getMultiDef() const { return MultiDef; }
   MultiBlockState getMultiBlock() const { return MultiBlock; }
-  const Inst *getFirstDefinition() const;
+  const Inst *getFirstDefinitionSingleBlock() const;
   const Inst *getSingleDefinition() const;
+  const Inst *getFirstDefinition() const;
   const InstDefList &getLatterDefinitions() const { return Definitions; }
   CfgNode *getNode() const { return SingleUseNode; }
   RegWeight getUseWeight() const { return UseWeight; }
@@ -643,11 +644,10 @@ private:
   MultiBlockState MultiBlock = MBS_Unknown;
   CfgNode *SingleUseNode = nullptr;
   CfgNode *SingleDefNode = nullptr;
-  /// All definitions of the variable are collected here, in increasing
-  /// order of instruction number.
+  /// All definitions of the variable are collected in Definitions[] (except for
+  /// the earliest definition), in increasing order of instruction number.
   InstDefList Definitions; /// Only used if Kind==VMK_All
-  const Inst *FirstOrSingleDefinition =
-      nullptr; /// Is a copy of Definitions[0] if Kind==VMK_All
+  const Inst *FirstOrSingleDefinition = nullptr;
   RegWeight UseWeight;
 };
 
@@ -665,6 +665,7 @@ public:
   /// Add a single node. This is called by init(), and can be called
   /// incrementally from elsewhere, e.g. after edge-splitting.
   void addNode(CfgNode *Node);
+  MetadataKind getKind() const { return Kind; }
   /// Returns whether the given Variable is tracked in this object. It should
   /// only return false if changes were made to the CFG after running init(), in
   /// which case the state is stale and the results shouldn't be trusted (but it
@@ -678,13 +679,17 @@ public:
   /// Returns the first definition instruction of the given Variable. This is
   /// only valid for variables whose definitions are all within the same block,
   /// e.g. T after the lowered sequence "T=B; T+=C; A=T", for which
-  /// getFirstDefinition(T) would return the "T=B" instruction. For variables
-  /// with definitions span multiple blocks, nullptr is returned.
-  const Inst *getFirstDefinition(const Variable *Var) const;
+  /// getFirstDefinitionSingleBlock(T) would return the "T=B" instruction. For
+  /// variables with definitions span multiple blocks, nullptr is returned.
+  const Inst *getFirstDefinitionSingleBlock(const Variable *Var) const;
   /// Returns the definition instruction of the given Variable, when the
   /// variable has exactly one definition. Otherwise, nullptr is returned.
   const Inst *getSingleDefinition(const Variable *Var) const;
-  /// Returns the list of all definition instructions of the given Variable.
+  /// getFirstDefinition() and getLatterDefinitions() are used together to
+  /// return the complete set of instructions that define the given Variable,
+  /// regardless of whether the definitions are within the same block (in
+  /// contrast to getFirstDefinitionSingleBlock).
+  const Inst *getFirstDefinition(const Variable *Var) const;
   const InstDefList &getLatterDefinitions(const Variable *Var) const;
 
   /// Returns whether the given Variable is live across multiple blocks. Mainly,

diff --git a/src/IceRegAlloc.cpp b/src/IceRegAlloc.cpp
index dacbc00..50e014a 100644 (file)
--- a/src/IceRegAlloc.cpp
+++ b/src/IceRegAlloc.cpp
@@ -38,9 +38,8 @@ bool overlapsDefs(const Cfg *Func, const Variable *Item, const Variable *Var) {
   if (const Inst *FirstDef = VMetadata->getFirstDefinition(Var))
     if (Item->getLiveRange().overlapsInst(FirstDef->getNumber(), UseTrimmed))
       return true;
-  const InstDefList &Defs = VMetadata->getLatterDefinitions(Var);
-  for (size_t i = 0; i < Defs.size(); ++i) {
-    if (Item->getLiveRange().overlapsInst(Defs[i]->getNumber(), UseTrimmed))
+  for (const Inst *Def : VMetadata->getLatterDefinitions(Var)) {
+    if (Item->getLiveRange().overlapsInst(Def->getNumber(), UseTrimmed))
       return true;
   }
   return false;
@@ -463,7 +462,8 @@ void LinearScan::findRegisterPreference(IterationState &Iter) {
 
   if (FindPreference) {
     VariablesMetadata *VMetadata = Func->getVMetadata();
-    if (const Inst *DefInst = VMetadata->getFirstDefinition(Iter.Cur)) {
+    if (const Inst *DefInst =
+            VMetadata->getFirstDefinitionSingleBlock(Iter.Cur)) {
       assert(DefInst->getDest() == Iter.Cur);
       bool IsAssign = DefInst->isSimpleAssign();
       bool IsSingleDef = !VMetadata->isMultiDef(Iter.Cur);

diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index fee1048..c1a5919 100644 (file)
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -2051,13 +2051,12 @@ private:
     return Context->getGlobalConstantByID(0);
   }
 
-  void verifyCallArgTypeMatches(Ice::FunctionDeclaration *Fcn,
-                                Ice::SizeT Index, Ice::Type ArgType,
-                                Ice::Type ParamType) {
+  void verifyCallArgTypeMatches(Ice::FunctionDeclaration *Fcn, Ice::SizeT Index,
+                                Ice::Type ArgType, Ice::Type ParamType) {
     if (ArgType != ParamType) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
-      StrBuf << "Argument " << (Index + 1)  << " of " << printName(Fcn)
+      StrBuf << "Argument " << (Index + 1) << " of " << printName(Fcn)
              << " expects " << ParamType << ". Found: " << ArgType;
       Error(StrBuf.str());
     }
@@ -2733,7 +2732,7 @@ void FunctionParser::ProcessRecord() {
       return;
 
     // Extract out the the call parameters.
-    SmallVector<Ice::Operand*, 8> Params;
+    SmallVector<Ice::Operand *, 8> Params;
     for (Ice::SizeT Index = ParamsStartIndex; Index < Values.size(); ++Index) {
       Ice::Operand *Op = getRelativeOperand(Values[Index], BaseIndex);
       if (isIRGenerationDisabled())
@@ -2741,8 +2740,8 @@ void FunctionParser::ProcessRecord() {
       if (Op == nullptr) {
         std::string Buffer;
         raw_string_ostream StrBuf(Buffer);
-        StrBuf << "Parameter " << (Index - ParamsStartIndex + 1)
-               << " of " << printName(Fcn) << " is not defined";
+        StrBuf << "Parameter " << (Index - ParamsStartIndex + 1) << " of "
+               << printName(Fcn) << " is not defined";
         Error(StrBuf.str());
         if (ReturnType != Ice::IceType_void)
           setNextLocalInstIndex(nullptr);
@@ -2755,8 +2754,8 @@ void FunctionParser::ProcessRecord() {
     if (IntrinsicInfo == nullptr && !isCallReturnType(ReturnType)) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
-      StrBuf << "Return type of " << printName(Fcn) << " is invalid: "
-             << ReturnType;
+      StrBuf << "Return type of " << printName(Fcn)
+             << " is invalid: " << ReturnType;
       Error(StrBuf.str());
       ReturnType = Ice::IceType_i32;
     }
@@ -2772,8 +2771,8 @@ void FunctionParser::ProcessRecord() {
       Ice::Operand *Op = Params[Index];
       Ice::Type OpType = Op->getType();
       if (Signature)
-        verifyCallArgTypeMatches(
-            Fcn, Index, OpType, Signature->getArgType(Index));
+        verifyCallArgTypeMatches(Fcn, Index, OpType,
+                                 Signature->getArgType(Index));
       if (IntrinsicInfo) {
         verifyCallArgTypeMatches(Fcn, Index, OpType,
                                  IntrinsicInfo->getArgType(Index));