Subzero: Fix incorrect address mode inference involving Phi temporaries.

src/IceInst.cpp

@@ -359,6 +359,7 @@ Inst *InstPhi::lower(Cfg *Func, CfgNode *Node) {
   assert(Dest);
   IceString PhiName = Dest->getName() + "_phi";
   Variable *NewSrc = Func->makeVariable(Dest->getType(), Node, PhiName);
+  NewSrc->setIsMultidef();
   this->Dest = NewSrc;
   InstAssign *NewInst = InstAssign::create(Func, Dest, NewSrc);
   // Set Dest and NewSrc to have affinity with each other, as a hint

src/IceOperand.h

@@ -349,6 +349,14 @@ public:
   bool isMultiblockLife() const { return (DefNode == NULL); }
   void setUse(const Inst *Inst, const CfgNode *Node);
 
+  // Multidef means a variable is non-SSA and has multiple defining
+  // instructions.  Currently this classification is limited to SSA
+  // lowering temporaries where the definitions are in different basic
+  // blocks, and it is not maintained during target lowering when the
+  // same temporary may be updated in consecutive instructions.
+  bool getIsMultidef() const { return IsMultidef; }
+  void setIsMultidef() { IsMultidef = true; }
+
   bool getIsArg() const { return IsArgument; }
   void setIsArg(Cfg *Func, bool IsArg = true);
 
@@ -419,9 +427,10 @@ public:
 private:
   Variable(Type Ty, const CfgNode *Node, SizeT Index, const IceString &Name)
       : Operand(kVariable, Ty), Number(Index), Name(Name), DefInst(NULL),
-        DefNode(Node), IsArgument(false), StackOffset(0), RegNum(NoRegister),
-        RegNumTmp(NoRegister), Weight(1), RegisterPreference(NULL),
-        AllowRegisterOverlap(false), LoVar(NULL), HiVar(NULL) {
+        DefNode(Node), IsMultidef(false), IsArgument(false), StackOffset(0),
+        RegNum(NoRegister), RegNumTmp(NoRegister), Weight(1),
+        RegisterPreference(NULL), AllowRegisterOverlap(false), LoVar(NULL),
+        HiVar(NULL) {
     Vars = VarsReal;
     Vars[0] = this;
     NumVars = 1;
@@ -443,6 +452,7 @@ private:
   // Cfg.  This saves space in the Variable, and removes the fragility
   // of incrementally computing and maintaining the information.
   const CfgNode *DefNode;
+  bool IsMultidef;
   bool IsArgument;
   // StackOffset is the canonical location on stack (only if
   // RegNum<0 || IsArgument).

src/IceTargetLoweringX8632.cpp

@@ -3547,71 +3547,80 @@ void dumpAddressOpt(const Cfg *Func, const Variable *Base,
   Str << ", Shift=" << Shift << ", Offset=" << Offset << "\n";
 }
 
-void computeAddressOpt(Cfg *Func, const Inst *Instr, Variable *&Base,
-                       Variable *&Index, uint16_t &Shift, int32_t &Offset) {
-  Func->setCurrentNode(NULL);
-  if (Func->getContext()->isVerbose(IceV_AddrOpt)) {
-    Ostream &Str = Func->getContext()->getStrDump();
-    Str << "\nStarting computeAddressOpt for instruction:\n  ";
-    Instr->dumpDecorated(Func);
-  }
-  (void)Offset; // TODO: pattern-match for non-zero offsets.
-  if (Base == NULL)
-    return;
-  // If the Base has more than one use or is live across multiple
-  // blocks, then don't go further.  Alternatively (?), never consider
-  // a transformation that would change a variable that is currently
-  // *not* live across basic block boundaries into one that *is*.
-  if (Base->isMultiblockLife() /* || Base->getUseCount() > 1*/)
-    return;
-
-  while (true) {
-    // Base is Base=Var ==>
-    //   set Base=Var
-    const Inst *BaseInst = Base->getDefinition();
-    Operand *BaseOperand0 = BaseInst ? BaseInst->getSrc(0) : NULL;
-    Variable *BaseVariable0 = llvm::dyn_cast_or_null<Variable>(BaseOperand0);
-    // TODO: Helper function for all instances of assignment
-    // transitivity.
-    if (BaseInst && llvm::isa<InstAssign>(BaseInst) && BaseVariable0 &&
-        // TODO: ensure BaseVariable0 stays single-BB
+bool matchTransitiveAssign(Variable *&Var, const Inst *&Reason) {
+  // Var originates from Var=SrcVar ==>
+  //   set Var:=SrcVar
+  if (Var == NULL)
+    return false;
+  if (const Inst *VarAssign = Var->getDefinition()) {
+    if (llvm::isa<InstAssign>(VarAssign)) {
+      Operand *SrcOp = VarAssign->getSrc(0);
+      assert(SrcOp);
+      if (Variable *SrcVar = llvm::dyn_cast<Variable>(SrcOp)) {
+        if (!SrcVar->getIsMultidef() &&
+            // TODO: ensure SrcVar stays single-BB
             true) {
-      Base = BaseVariable0;
-      dumpAddressOpt(Func, Base, Index, Shift, Offset, BaseInst);
-      continue;
+          Var = SrcVar;
+          Reason = VarAssign;
+          return true;
         }
+      }
+    }
+  }
+  return false;
+}
 
-    // Index is Index=Var ==>
-    //   set Index=Var
-
+bool matchCombinedBaseIndex(Variable *&Base, Variable *&Index, uint16_t &Shift,
+                            const Inst *&Reason) {
   // Index==NULL && Base is Base=Var1+Var2 ==>
   //   set Base=Var1, Index=Var2, Shift=0
-    Operand *BaseOperand1 =
-        BaseInst && BaseInst->getSrcSize() >= 2 ? BaseInst->getSrc(1) : NULL;
-    Variable *BaseVariable1 = llvm::dyn_cast_or_null<Variable>(BaseOperand1);
-    if (Index == NULL && isAdd(BaseInst) && BaseVariable0 && BaseVariable1 &&
-        // TODO: ensure BaseVariable0 and BaseVariable1 stay single-BB
+  if (Base == NULL)
+    return false;
+  if (Index != NULL)
+    return false;
+  const Inst *BaseInst = Base->getDefinition();
+  if (BaseInst == NULL)
+    return false;
+  if (BaseInst->getSrcSize() < 2)
+    return false;
+  if (Variable *Var1 = llvm::dyn_cast<Variable>(BaseInst->getSrc(0))) {
+    if (Var1->getIsMultidef())
+      return false;
+    if (Variable *Var2 = llvm::dyn_cast<Variable>(BaseInst->getSrc(1))) {
+      if (Var2->getIsMultidef())
+        return false;
+      if (isAdd(BaseInst) &&
+          // TODO: ensure Var1 and Var2 stay single-BB
           true) {
-      Base = BaseVariable0;
-      Index = BaseVariable1;
+        Base = Var1;
+        Index = Var2;
         Shift = 0; // should already have been 0
-      dumpAddressOpt(Func, Base, Index, Shift, Offset, BaseInst);
-      continue;
+        Reason = BaseInst;
+        return true;
+      }
     }
+  }
+  return false;
+}
 
+bool matchShiftedIndex(Variable *&Index, uint16_t &Shift, const Inst *&Reason) {
   // Index is Index=Var*Const && log2(Const)+Shift<=3 ==>
   //   Index=Var, Shift+=log2(Const)
-    const Inst *IndexInst = Index ? Index->getDefinition() : NULL;
+  if (Index == NULL)
+    return false;
+  const Inst *IndexInst = Index->getDefinition();
+  if (IndexInst == NULL)
+    return false;
+  if (IndexInst->getSrcSize() < 2)
+    return false;
   if (const InstArithmetic *ArithInst =
-            llvm::dyn_cast_or_null<InstArithmetic>(IndexInst)) {
-      Operand *IndexOperand0 = ArithInst->getSrc(0);
-      Variable *IndexVariable0 = llvm::dyn_cast<Variable>(IndexOperand0);
-      Operand *IndexOperand1 = ArithInst->getSrc(1);
-      ConstantInteger *IndexConstant1 =
-          llvm::dyn_cast<ConstantInteger>(IndexOperand1);
-      if (ArithInst->getOp() == InstArithmetic::Mul && IndexVariable0 &&
-          IndexOperand1->getType() == IceType_i32 && IndexConstant1) {
-        uint64_t Mult = IndexConstant1->getValue();
+          llvm::dyn_cast<InstArithmetic>(IndexInst)) {
+    if (Variable *Var = llvm::dyn_cast<Variable>(ArithInst->getSrc(0))) {
+      if (ConstantInteger *Const =
+              llvm::dyn_cast<ConstantInteger>(ArithInst->getSrc(1))) {
+        if (ArithInst->getOp() == InstArithmetic::Mul &&
+            !Var->getIsMultidef() && Const->getType() == IceType_i32) {
+          uint64_t Mult = Const->getValue();
           uint32_t LogMult;
           switch (Mult) {
           case 1:
@@ -3627,26 +3636,36 @@ void computeAddressOpt(Cfg *Func, const Inst *Instr, Variable *&Base,
             LogMult = 3;
             break;
           default:
-          LogMult = 4;
-          break;
+            return false;
           }
           if (Shift + LogMult <= 3) {
-          Index = IndexVariable0;
+            Index = Var;
             Shift += LogMult;
-          dumpAddressOpt(Func, Base, Index, Shift, Offset, IndexInst);
-          continue;
+            Reason = IndexInst;
+            return true;
+          }
         }
       }
     }
+  }
+  return false;
+}
 
+bool matchOffsetBase(Variable *&Base, int32_t &Offset, const Inst *&Reason) {
   // Base is Base=Var+Const || Base is Base=Const+Var ==>
   //   set Base=Var, Offset+=Const
   // Base is Base=Var-Const ==>
   //   set Base=Var, Offset-=Const
-    const InstArithmetic *ArithInst =
-        llvm::dyn_cast_or_null<const InstArithmetic>(BaseInst);
-    if (ArithInst && (ArithInst->getOp() == InstArithmetic::Add ||
-                      ArithInst->getOp() == InstArithmetic::Sub)) {
+  if (Base == NULL)
+    return false;
+  const Inst *BaseInst = Base->getDefinition();
+  if (BaseInst == NULL)
+    return false;
+  if (const InstArithmetic *ArithInst =
+          llvm::dyn_cast<const InstArithmetic>(BaseInst)) {
+    if (ArithInst->getOp() != InstArithmetic::Add &&
+        ArithInst->getOp() != InstArithmetic::Sub)
+      return false;
     bool IsAdd = ArithInst->getOp() == InstArithmetic::Add;
     Variable *Var = NULL;
     ConstantInteger *Const = NULL;
@@ -3658,13 +3677,45 @@ void computeAddressOpt(Cfg *Func, const Inst *Instr, Variable *&Base,
       Const = llvm::dyn_cast<ConstantInteger>(ArithInst->getSrc(0));
       Var = llvm::dyn_cast<Variable>(ArithInst->getSrc(1));
     }
-      if (!(Const && Var)) {
-        break;
-      }
+    if (Var == NULL || Const == NULL || Var->getIsMultidef())
+      return false;
     Base = Var;
     Offset += IsAdd ? Const->getValue() : -Const->getValue();
-      dumpAddressOpt(Func, Base, Index, Shift, Offset, BaseInst);
-      continue;
+    Reason = BaseInst;
+    return true;
+  }
+  return false;
+}
+
+void computeAddressOpt(Cfg *Func, const Inst *Instr, Variable *&Base,
+                       Variable *&Index, uint16_t &Shift, int32_t &Offset) {
+  Func->setCurrentNode(NULL);
+  if (Func->getContext()->isVerbose(IceV_AddrOpt)) {
+    Ostream &Str = Func->getContext()->getStrDump();
+    Str << "\nStarting computeAddressOpt for instruction:\n  ";
+    Instr->dumpDecorated(Func);
+  }
+  (void)Offset; // TODO: pattern-match for non-zero offsets.
+  if (Base == NULL)
+    return;
+  // If the Base has more than one use or is live across multiple
+  // blocks, then don't go further.  Alternatively (?), never consider
+  // a transformation that would change a variable that is currently
+  // *not* live across basic block boundaries into one that *is*.
+  if (Base->isMultiblockLife() /* || Base->getUseCount() > 1*/)
+    return;
+
+  bool Continue = true;
+  while (Continue) {
+    const Inst *Reason = NULL;
+    if (matchTransitiveAssign(Base, Reason) ||
+        matchTransitiveAssign(Index, Reason) ||
+        matchCombinedBaseIndex(Base, Index, Shift, Reason) ||
+        matchShiftedIndex(Index, Shift, Reason) ||
+        matchOffsetBase(Base, Offset, Reason)) {
+      dumpAddressOpt(Func, Base, Index, Shift, Offset, Reason);
+    } else {
+      Continue = false;
     }
 
     // Index is Index=Var<<Const && Const+Shift<=3 ==>
@@ -3688,7 +3739,6 @@ void computeAddressOpt(Cfg *Func, const Inst *Instr, Variable *&Base,
 
     // TODO: consider overflow issues with respect to Offset.
     // TODO: handle symbolic constants.
-    break;
   }
 }
 

tests_lit/llvm2ice_tests/phi.ll

@@ -58,3 +58,53 @@ target:
 
 ; ERRORS-NOT: ICE translation error
 ; DUMP-NOT: SZ
+
+; Test that address mode inference doesn't extend past
+; multi-definition, non-SSA Phi temporaries.
+define internal i32 @testPhi3(i32 %arg) {
+entry:
+  br label %body
+body:
+  %merge = phi i32 [ %arg, %entry ], [ %elt, %body ]
+  %interior = add i32 %merge, 1000
+  %__4 = inttoptr i32 %interior to i32*
+  %elt = load i32* %__4, align 1
+  %cmp = icmp eq i32 %elt, 0
+  br i1 %cmp, label %exit, label %body
+exit:
+  %__6 = inttoptr i32 %interior to i32*
+  store i32 %arg, i32* %__6, align 1
+  ret i32 %arg
+}
+; I can't figure out how to reliably test this for correctness, so I
+; will just include patterns for the entire current O2 sequence.  This
+; may need to be changed when meaningful optimizations are added.
+; The key is to avoid the "bad" pattern like this:
+;
+; testPhi3:
+; .LtestPhi3$entry:
+;         mov     eax, dword ptr [esp+4]
+;         mov     ecx, eax
+; .LtestPhi3$body:
+;         mov     ecx, dword ptr [ecx+1000]
+;         cmp     ecx, 0
+;         jne     .LtestPhi3$body
+; .LtestPhi3$exit:
+;         mov     dword ptr [ecx+1000], eax
+;         ret
+;
+; This is bad because the final store address is supposed to be the
+; same as the load address in the loop, but it has clearly been
+; over-optimized into a null pointer dereference.
+
+; CHECK-LABEL: testPhi3
+; CHECK: push [[EBX:.*]]
+; CHECK: mov {{.*}}, dword ptr [esp
+; CHECK: jmp
+; CHECK: mov
+; CHECK: mov {{.*}}[[ADDR:.*1000]]
+; CHECK: cmp {{.*}}, 0
+; CHECK: je
+; CHECK: jmp
+; CHECK: mov {{.*}}[[ADDR]]
+; CHECK: pop [[EBX]]