Subzero: Consider all instruction variables for register preference.

src/IceInst.cpp

@@ -249,6 +249,8 @@ InstAssign::InstAssign(Cfg *Func, Variable *Dest, Operand *Source)
   addSource(Source);
 }
 
+bool InstAssign::isVarAssign() const { return llvm::isa<Variable>(getSrc(0)); }
+
 // If TargetTrue==TargetFalse, we turn it into an unconditional branch. This
 // ensures that, along with the 'switch' instruction semantics, there is at
 // most one edge from one node to another.

src/IceInst.h

@@ -127,7 +127,9 @@ public:
     return false;
   }
 
-  virtual bool isSimpleAssign() const { return false; }
+  /// Returns true if the instruction is equivalent to a simple
+  /// "var_dest=var_src" assignment where the dest and src are both variables.
+  virtual bool isVarAssign() const { return false; }
 
   void livenessLightweight(Cfg *Func, LivenessBV &Live);
   /// Calculates liveness for this instruction. Returns true if this instruction
@@ -313,7 +315,7 @@ public:
   static InstAssign *create(Cfg *Func, Variable *Dest, Operand *Source) {
     return new (Func->allocate<InstAssign>()) InstAssign(Func, Dest, Source);
   }
-  bool isSimpleAssign() const override { return true; }
+  bool isVarAssign() const override;
   void dump(const Cfg *Func) const override;
   static bool classof(const Inst *Inst) { return Inst->getKind() == Assign; }
 

src/IceInstARM32.h

@@ -1152,7 +1152,7 @@ public:
     return !isMultiDest() && !isMultiSource() &&
            checkForRedundantAssign(getDest(), getSrc(0));
   }
-  bool isSimpleAssign() const override { return true; }
+  bool isVarAssign() const override { return llvm::isa<Variable>(getSrc(0)); }
   void emit(const Cfg *Func) const override;
   void emitIAS(const Cfg *Func) const override;
   void dump(const Cfg *Func) const override;

src/IceInstVarIter.h

@@ -145,8 +145,8 @@
 #define FOREACH_VAR_IN_INST(Var, Instr)                                        \
   for (SizeT Sz_I##Var##_ = 0, Sz_##Var##Index_ = 0,                           \
              Sz_SrcSize##Var##_ = (Instr).getSrcSize(), Sz_J##Var##_ = 0,      \
-             Sz_NumVars##Var##_ = 0;                                           \
-       Sz_I##Var##_ < Sz_SrcSize##Var##_; ++Sz_I##Var##_)                      \
+             Sz_NumVars##Var##_ = 0, Sz_Foreach_Break = 0;                     \
+       !Sz_Foreach_Break && Sz_I##Var##_ < Sz_SrcSize##Var##_; ++Sz_I##Var##_) \
     if (Operand *Sz_Op##Var##_ = nullptr)                                      \
       /*nothing*/;                                                             \
     else                                                                       \
@@ -154,7 +154,7 @@
                (Sz_J##Var##_ = 0,                                              \
                Sz_Op##Var##_ = (Instr).getSrc(Sz_I##Var##_),                   \
                Sz_NumVars##Var##_ = Sz_Op##Var##_->getNumVars(), nullptr);     \
-           Sz_J##Var##_ < Sz_NumVars##Var##_ &&                                \
+           !Sz_Foreach_Break && Sz_J##Var##_ < Sz_NumVars##Var##_ &&           \
            ((Var = Sz_Op##Var##_->getVar(Sz_J##Var##_)), true);                \
            ++Sz_J##Var##_, ++Sz_##Var##Index_)
 
@@ -162,6 +162,12 @@
   (static_cast<const SizeT>(Sz_##V##_))
 #define IndexOfVarInInst(Var) IsOnlyValidInFOREACH_VAR_IN_INST(Var##Index)
 #define IndexOfVarOperandInInst(Var) IsOnlyValidInFOREACH_VAR_IN_INST(I##Var)
+#define FOREACH_VAR_IN_INST_BREAK                                              \
+  if (true) {                                                                  \
+    Sz_Foreach_Break = 1;                                                      \
+    continue;                                                                  \
+  } else {                                                                     \
+  }
 #undef OnlyValidIn_FOREACH_VAR_IN_INSTS
 
 #endif // SUBZERO_SRC_ICEINSTVARITER_H

src/IceInstX86Base.h

@@ -977,7 +977,9 @@ public:
   bool isRedundantAssign() const override {
     return checkForRedundantAssign(this->getDest(), this->getSrc(0));
   }
-  bool isSimpleAssign() const override { return true; }
+  bool isVarAssign() const override {
+    return llvm::isa<Variable>(this->getSrc(0));
+  }
   void dump(const Cfg *Func) const override {
     if (!BuildDefs::dump())
       return;

src/IceRegAlloc.cpp

@@ -493,20 +493,18 @@ void LinearScan::findRegisterPreference(IterationState &Iter) {
     if (const Inst *DefInst =
             VMetadata->getFirstDefinitionSingleBlock(Iter.Cur)) {
       assert(DefInst->getDest() == Iter.Cur);
-      bool IsAssign = DefInst->isSimpleAssign();
+      bool IsAssign = DefInst->isVarAssign();
       bool IsSingleDef = !VMetadata->isMultiDef(Iter.Cur);
-      for (SizeT i = 0; i < DefInst->getSrcSize(); ++i) {
-        // TODO(stichnot): Iterate through the actual Variables of the
-        // instruction, not just the source operands. This could capture Load
-        // instructions, including address mode optimization, for Prefer (but
-        // not for AllowOverlap).
-        if (Variable *SrcVar = llvm::dyn_cast<Variable>(DefInst->getSrc(i))) {
-          int32_t SrcReg = SrcVar->getRegNumTmp();
-          // Only consider source variables that have (so far) been assigned a
-          // register. That register must be one in the RegMask set, e.g. don't
-          // try to prefer the stack pointer as a result of the stacksave
-          // intrinsic.
-          if (SrcVar->hasRegTmp() && Iter.RegMask[SrcReg]) {
+      FOREACH_VAR_IN_INST(SrcVar, *DefInst) {
+        // Only consider source variables that have (so far) been assigned a
+        // register. That register must be one in the RegMask set, e.g. don't
+        // try to prefer the stack pointer as a result of the stacksave
+        // intrinsic.
+        if (SrcVar->hasRegTmp()) {
+          const int32_t SrcReg = SrcVar->getRegNumTmp();
+          const bool IsAliasAvailable =
+              (Iter.RegMask & *RegAliases[SrcReg]).any();
+          if (IsAliasAvailable) {
             if (FindOverlap && !Iter.Free[SrcReg]) {
               // Don't bother trying to enable AllowOverlap if the register is
               // already free.
@@ -516,6 +514,14 @@ void LinearScan::findRegisterPreference(IterationState &Iter) {
             if (Iter.AllowOverlap || Iter.Free[SrcReg]) {
               Iter.Prefer = SrcVar;
               Iter.PreferReg = SrcReg;
+              // Stop looking for a preference after the first valid preference
+              // is found.  One might think that we should look at all
+              // instruction variables to find the best <Prefer,AllowOverlap>
+              // combination, but note that AllowOverlap can only be true for a
+              // simple assignment statement which can have only one source
+              // operand, so it's not possible for AllowOverlap to be true
+              // beyond the first source operand.
+              FOREACH_VAR_IN_INST_BREAK;
             }
           }
         }
@@ -539,6 +545,7 @@ void LinearScan::filterFreeWithInactiveRanges(IterationState &Iter) {
     if (!Item->rangeOverlaps(Iter.Cur))
       continue;
     const llvm::SmallBitVector &Aliases = *RegAliases[Item->getRegNumTmp()];
+    // TODO(stichnot): Do this with bitvector ops, not a loop, for efficiency.
     for (int32_t RegAlias = Aliases.find_first(); RegAlias >= 0;
          RegAlias = Aliases.find_next(RegAlias)) {
       // Don't assert(Free[RegNum]) because in theory (though probably never in
@@ -852,6 +859,8 @@ void LinearScan::scan(const llvm::SmallBitVector &RegMaskFull,
     if (Iter.AllowOverlap) {
       for (const Variable *Item : Active) {
         int32_t RegNum = Item->getRegNumTmp();
+        // TODO(stichnot): Consider aliases of RegNum.  This is probably a
+        // correctness issue.
         if (Item != Iter.Prefer && RegNum == Iter.PreferReg &&
             overlapsDefs(Func, Iter.Cur, Item)) {
           Iter.AllowOverlap = false;

src/IceTargetLowering.cpp

@@ -81,12 +81,11 @@ void LoweringContext::availabilityUpdate() {
   Inst *Instr = LastInserted;
   if (Instr == nullptr)
     return;
-  if (!Instr->isSimpleAssign())
+  if (!Instr->isVarAssign())
     return;
-  if (auto *SrcVar = llvm::dyn_cast<Variable>(Instr->getSrc(0))) {
-    LastDest = Instr->getDest();
-    LastSrc = SrcVar;
-  }
+  // Since isVarAssign() is true, the source operand must be a Variable.
+  LastDest = Instr->getDest();
+  LastSrc = llvm::cast<Variable>(Instr->getSrc(0));
 }
 
 Variable *LoweringContext::availabilityGet(Operand *Src) const {