Subzero. ARM32. Removing memory legalization warts.

src/IceTargetLoweringARM32.cpp

Index: src/IceTargetLoweringARM32.cpp
diff --git a/src/IceTargetLoweringARM32.cpp b/src/IceTargetLoweringARM32.cpp
index 9657c1dd0c28a25ddf0b42a30fbc90b617e970e5..389fe01e78afc58e79395f1373534aa1e2469dd3 100644
--- a/src/IceTargetLoweringARM32.cpp
+++ b/src/IceTargetLoweringARM32.cpp
@@ -475,9 +475,11 @@ void TargetARM32::emitVariable(const Variable *Var) const {
       Offset += getStackAdjustment();
   }
   const Type VarTy = Var->getType();
+  /*

[Karl, 2015/11/18 18:40:42]

Remove comment?

[John, 2015/11/19 13:41:22]

Oops, done.

   if (!isLegalMemOffset(VarTy, Offset)) {
-    llvm::report_fatal_error("Illegal stack offset");
+    // llvm::report_fatal_error("Illegal stack offset");
   }
+  */
   Str << "[" << getRegName(BaseRegNum, VarTy);
   if (Offset != 0) {
     Str << ", " << getConstantPrefix() << Offset;
@@ -625,7 +627,7 @@ void TargetARM32::lowerArguments() {
 // to copy Arg into its assigned register if applicable.
 void TargetARM32::finishArgumentLowering(Variable *Arg, Variable *FramePtr,
                                          size_t BasicFrameOffset,
-                                         size_t &InArgsSizeBytes) {
+                                         size_t *InArgsSizeBytes) {
   if (auto *Arg64On32 = llvm::dyn_cast<Variable64On32>(Arg)) {
     Variable *Lo = Arg64On32->getLo();
     Variable *Hi = Arg64On32->getHi();
@@ -634,26 +636,23 @@ void TargetARM32::finishArgumentLowering(Variable *Arg, Variable *FramePtr,
     return;
   }
   Type Ty = Arg->getType();
-  InArgsSizeBytes = applyStackAlignmentTy(InArgsSizeBytes, Ty);
-  Arg->setStackOffset(BasicFrameOffset + InArgsSizeBytes);
-  InArgsSizeBytes += typeWidthInBytesOnStack(Ty);
-  // If the argument variable has been assigned a register, we need to load the
-  // value from the stack slot.
-  if (Arg->hasReg()) {
-    assert(Ty != IceType_i64);
-    // This should be simple, just load the parameter off the stack using a nice
-    // sp + imm addressing mode. Because ARM, we can't do that (e.g., VLDR, for
-    // fp types, cannot have an index register), so we legalize the memory
-    // operand instead.
-    auto *Mem = OperandARM32Mem::create(
-        Func, Ty, FramePtr, llvm::cast<ConstantInteger32>(
-                                Ctx->getConstantInt32(Arg->getStackOffset())));
-    _mov(Arg, legalizeToReg(Mem, Arg->getRegNum()));
-    // This argument-copying instruction uses an explicit OperandARM32Mem
-    // operand instead of a Variable, so its fill-from-stack operation has to
-    // be tracked separately for statistics.
-    Ctx->statsUpdateFills();
+  assert(Ty != IceType_i64);
+
+  *InArgsSizeBytes = applyStackAlignmentTy(*InArgsSizeBytes, Ty);
+  const int32_t ArgStackOffset = BasicFrameOffset + *InArgsSizeBytes;
+  *InArgsSizeBytes += typeWidthInBytesOnStack(Ty);
+
+  if (!Arg->hasReg()) {
+    Arg->setStackOffset(ArgStackOffset);
+    return;
   }
+
+  // If the argument variable has been assigned a register, we need to copy the
+  // value from the stack slot.
+  Variable *Parameter = Func->makeVariable(Ty);
+  Parameter->setMustNotHaveReg();
+  Parameter->setStackOffset(ArgStackOffset);
+  _mov(Arg, Parameter);
 }
 
 Type TargetARM32::stackSlotType() { return IceType_i32; }
@@ -754,8 +753,6 @@ void TargetARM32::addProlog(CfgNode *Node) {
       continue;
     }
     if (CalleeSaves[i] && RegsUsed[i]) {
-      // TODO(jvoung): do separate vpush for each floating point register
-      // segment and += 4, or 8 depending on type.
       ++NumCallee;
       Variable *PhysicalRegister = getPhysicalRegister(i);
       PreservedRegsSizeBytes +=
@@ -837,7 +834,7 @@ void TargetARM32::addProlog(CfgNode *Node) {
       InRegs = CC.I32InReg(&DummyReg);
     }
     if (!InRegs)
-      finishArgumentLowering(Arg, FramePtr, BasicFrameOffset, InArgsSizeBytes);
+      finishArgumentLowering(Arg, FramePtr, BasicFrameOffset, &InArgsSizeBytes);
   }
 
   // Fill in stack offsets for locals.
@@ -981,11 +978,18 @@ Variable *TargetARM32::newBaseRegister(int32_t OriginalOffset,
   return ScratchReg;
 }
 
-StackVariable *TargetARM32::legalizeStackSlot(Type Ty, int32_t Offset,
-                                              int32_t StackAdjust,
-                                              Variable *OrigBaseReg,
-                                              Variable **NewBaseReg,
-                                              int32_t *NewBaseOffset) {
+OperandARM32Mem *TargetARM32::createMemOperand(Type Ty, int32_t Offset,
+                                               int32_t StackAdjust,
+                                               Variable *OrigBaseReg,
+                                               Variable **NewBaseReg,
+                                               int32_t *NewBaseOffset) {
+  if (isLegalMemOffset(Ty, Offset + StackAdjust)) {
+    return OperandARM32Mem::create(
+        Func, Ty, OrigBaseReg, llvm::cast<ConstantInteger32>(
+                                   Ctx->getConstantInt32(Offset + StackAdjust)),
+        OperandARM32Mem::Offset);
+  }
+
   if (*NewBaseReg == nullptr) {
     *NewBaseReg = newBaseRegister(Offset, StackAdjust, OrigBaseReg);
     *NewBaseOffset = Offset + StackAdjust;
@@ -998,18 +1002,15 @@ StackVariable *TargetARM32::legalizeStackSlot(Type Ty, int32_t Offset,
     OffsetDiff = 0;
   }
 
-  StackVariable *NewDest = Func->makeVariable<StackVariable>(Ty);
-  NewDest->setMustNotHaveReg();
-  NewDest->setBaseRegNum((*NewBaseReg)->getRegNum());
-  NewDest->setStackOffset(OffsetDiff);
-  return NewDest;
+  return OperandARM32Mem::create(
+      Func, Ty, *NewBaseReg,
+      llvm::cast<ConstantInteger32>(Ctx->getConstantInt32(OffsetDiff)),
+      OperandARM32Mem::Offset);
 }
 
-void TargetARM32::legalizeMovStackAddrImm(InstARM32Mov *MovInstr,
-                                          int32_t StackAdjust,
-                                          Variable *OrigBaseReg,
-                                          Variable **NewBaseReg,
-                                          int32_t *NewBaseOffset) {
+void TargetARM32::legalizeMov(InstARM32Mov *MovInstr, int32_t StackAdjust,
+                              Variable *OrigBaseReg, Variable **NewBaseReg,
+                              int32_t *NewBaseOffset) {
   Variable *Dest = MovInstr->getDest();
   assert(Dest != nullptr);
   Type DestTy = Dest->getType();
@@ -1017,6 +1018,7 @@ void TargetARM32::legalizeMovStackAddrImm(InstARM32Mov *MovInstr,
 
   Operand *Src = MovInstr->getSrc(0);
   Type SrcTy = Src->getType();
+  (void)SrcTy;
   assert(SrcTy != IceType_i64);
 
   if (MovInstr->isMultiDest() || MovInstr->isMultiSource())
@@ -1024,40 +1026,29 @@ void TargetARM32::legalizeMovStackAddrImm(InstARM32Mov *MovInstr,
 
   bool Legalized = false;
   if (!Dest->hasReg()) {
-    assert(llvm::cast<Variable>(Src)->hasReg());
+    auto *SrcR = llvm::cast<Variable>(Src);

[Karl, 2015/11/18 18:40:42]

const auto?

[John, 2015/11/19 13:41:22]

done.

+    assert(SrcR->hasReg());
     const int32_t Offset = Dest->getStackOffset();
-    if (!isLegalMemOffset(DestTy, Offset + StackAdjust)) {
-      Legalized = true;
-      Dest = legalizeStackSlot(DestTy, Offset, StackAdjust, OrigBaseReg,
-                               NewBaseReg, NewBaseOffset);
-    }
+    // This is a _mov(Mem(), Variable), i.e., a store.
+    _str(SrcR, createMemOperand(DestTy, Offset, StackAdjust, OrigBaseReg,
+                                NewBaseReg, NewBaseOffset),
+         MovInstr->getPredicate());
+    // _str() does not have a Dest, so we add a fake-def(Dest).
+    Context.insert(InstFakeDef::create(Func, Dest));
+    Legalized = true;
   } else if (auto *Var = llvm::dyn_cast<Variable>(Src)) {
     if (!Var->hasReg()) {
       const int32_t Offset = Var->getStackOffset();
-      if (!isLegalMemOffset(SrcTy, Offset + StackAdjust)) {
-        Legalized = true;
-        Src = legalizeStackSlot(SrcTy, Offset, StackAdjust, OrigBaseReg,
-                                NewBaseReg, NewBaseOffset);
-      }
-    }
-  } else if (auto *Mem = llvm::dyn_cast<OperandARM32Mem>(Src)) {
-    if (ConstantInteger32 *OffsetOp = Mem->getOffset()) {
-      const int32_t Offset = OffsetOp->getValue();
-      if (!isLegalMemOffset(SrcTy, Offset + StackAdjust)) {
-        assert(Mem->getBase()->hasReg());
-        assert(Mem->getBase()->getRegNum() == (int32_t)getFrameOrStackReg());
-        Legalized = true;
-        Src = legalizeStackSlot(SrcTy, Offset, StackAdjust, OrigBaseReg,
-                                NewBaseReg, NewBaseOffset);
-      }
+      _ldr(Dest, createMemOperand(DestTy, Offset, StackAdjust, OrigBaseReg,
+                                  NewBaseReg, NewBaseOffset),
+           MovInstr->getPredicate());
+      Legalized = true;
     }
   }
 
   if (Legalized) {
     if (MovInstr->isDestRedefined()) {
-      _mov_redefined(Dest, Src, MovInstr->getPredicate());
-    } else {
-      _mov(Dest, Src, MovInstr->getPredicate());
+      _set_dest_redefined();
     }
     MovInstr->setDeleted();
   }
@@ -1075,11 +1066,6 @@ void TargetARM32::legalizeStackSlots() {
   // clobber the flags register.
   Func->dump("Before legalizeStackSlots");
   assert(hasComputedFrame());
-  // Early exit, if SpillAreaSizeBytes is really small.
-  // TODO(jpp): this is not safe -- loads and stores of q registers can't have
-  // offsets.
-  if (isLegalMemOffset(IceType_v4i32, SpillAreaSizeBytes))
-    return;
   Variable *OrigBaseReg = getPhysicalRegister(getFrameOrStackReg());
   int32_t StackAdjust = 0;
   // Do a fairly naive greedy clustering for now. Pick the first stack slot
@@ -1122,11 +1108,9 @@ void TargetARM32::legalizeStackSlots() {
         }
       }
 
-      // The Lowering ensures that ldr and str always have legal Mem operands.
-      // The only other instruction that may access memory is mov.
       if (auto *MovInstr = llvm::dyn_cast<InstARM32Mov>(CurInstr)) {
-        legalizeMovStackAddrImm(MovInstr, StackAdjust, OrigBaseReg, &NewBaseReg,
-                                &NewBaseOffset);
+        legalizeMov(MovInstr, StackAdjust, OrigBaseReg, &NewBaseReg,
+                    &NewBaseOffset);
       }
     }
   }
@@ -4600,7 +4584,11 @@ Variable *TargetARM32::makeVectorOfZeros(Type Ty, int32_t RegNum) {
 Variable *TargetARM32::copyToReg(Operand *Src, int32_t RegNum) {
   Type Ty = Src->getType();
   Variable *Reg = makeReg(Ty, RegNum);
-  _mov(Reg, Src);
+  if (auto *Mem = llvm::dyn_cast<OperandARM32Mem>(Src)) {
+    _ldr(Reg, Mem);
+  } else {
+    _mov(Reg, Src);
+  }
   return Reg;
 }
 
@@ -4653,8 +4641,7 @@ Operand *TargetARM32::legalize(Operand *From, LegalMask Allowed,
       static constexpr bool ZeroExt = false;
       assert(MemTraits[Ty].CanHaveImm);
       if (!OperandARM32Mem::canHoldOffset(Ty, ZeroExt, Offset->getValue())) {
-        assert(RegBase->hasReg());
-        assert(RegBase->getRegNum() == (int32_t)getFrameOrStackReg());
+        assert(false);

[Karl, 2015/11/18 18:40:42]

Why have code below this? Doesn't assert always fail (except for release
compile, which assumes this won't happen)?

[John, 2015/11/19 13:41:22]

It used to be the case that TargetARM32::legalizeMov() would take care of these.
This assert was just a "heads up" to me, during development.

These were introduced by TargetARM32::finishArgumentLowering(). I hae changed
the implementation so that these are now emitted as _mov(Var, Mem()), which is
later converted to _ldr(Var, Mem()) during the mov legalization.

         // We are a bit more lenient with invalid immediate when accessing the
         // stack here, and then rely on legalizeStackSlots() to fix things as
         // appropriate.