Fix ARM assembler to pop registers in reverse order of pushes.

src/IceInstARM32.cpp

 //===- subzero/src/IceInstARM32.cpp - ARM32 instruction implementation ----===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 880 matching lines @@
   for (SizeT i = 0; i < RegCount; ++i) {
     if (IsFirst)
       IsFirst = false;
     else
       Str << ", ";
     Str << RegARM32::getRegName(Base + i);
   }
   Str << "}";
 }
 
-namespace {
-
-bool isAssignedConsecutiveRegisters(const Variable *Before,
-                                    const Variable *After) {
-  assert(Before->hasReg());
-  assert(After->hasReg());
-  return Before->getRegNum() + 1 == After->getRegNum();
-}
-
-} // end of anonymous namespace
-
 void InstARM32RegisterStackOp::emitUsingForm(const Cfg *Func,
                                              const EmitForm Form) const {
   SizeT NumRegs = getNumStackRegs();
   assert(NumRegs);
 
   const auto *Reg = llvm::cast<Variable>(getStackReg(0));
   if (isScalarIntegerType(Reg->getType())) {
     // Pop GPR registers.
     SizeT IntegerCount = 0;
     ARM32::IValueT GPRegisters = 0;
     const Variable *LastDest = nullptr;
     for (SizeT i = 0; i < NumRegs; ++i) {
       const Variable *Var = getStackReg(i);
       assert(Var->hasReg() && "stack op only applies to registers");
       int32_t Reg = RegARM32::getEncodedGPR(Var->getRegNum());
       LastDest = Var;
       GPRegisters |= (1 << Reg);
       ++IntegerCount;
     }
     if (IntegerCount == 1) {
       emitSingleGPR(Func, Form, LastDest);
     } else {
       emitMultipleGPRs(Func, Form, GPRegisters);
     }
   } else {
     // Pop vector/floating point registers.
     const Variable *BaseReg = nullptr;
     SizeT RegCount = 0;
     for (SizeT i = 0; i < NumRegs; ++i) {
-      const Variable *NextReg = getStackReg(i);
+      const Variable *NextReg = getStackSReg(i, NumRegs);
+      assert(NextReg->hasReg());
       if (BaseReg == nullptr) {
         BaseReg = NextReg;
         RegCount = 1;
       } else if (RegCount < VpushVpopMaxConsecRegs &&
                  isAssignedConsecutiveRegisters(Reg, NextReg)) {
+        BaseReg = updateBaseReg(BaseReg, NextReg);
         ++RegCount;
       } else {
         emitSRegs(Func, Form, BaseReg, RegCount);
         if (Form == Emit_Text && BuildDefs::dump()) {
           startNextInst(Func);
           Func->getContext()->getStrEmit() << "\n";
         }
         BaseReg = NextReg;
         RegCount = 1;
       }
@@ skipping 816 matching lines @@
 }
 
 const char *InstARM32Pop::getGPROpcode() const { return "pop"; }
 
 const char *InstARM32Pop::getSRegOpcode() const { return "vpop"; }
 
 Variable *InstARM32Pop::getStackReg(SizeT Index) const { return Dests[Index]; }
 
 SizeT InstARM32Pop::getNumStackRegs() const { return Dests.size(); }
 
+bool InstARM32Pop::isAssignedConsecutiveRegisters(const Variable *Before,
+                                                  const Variable *After) const {
+  return Before->getRegNum() - 1 == After->getRegNum();

[Jim Stichnoth, 2016/02/05 00:48:17]

I realize this is just transplanted code, but...

This code is relying on a certain ordering of the register enum values as laid
out in the .def file.  I want to be able to change/scramble the order and have
the generated code continue to work (though not necessarily being precisely as
efficient).

I think to be safe here, you should actually be comparing the encoded values,
not the enum values.

[John, 2016/02/05 01:46:25]

The only issue with encoded reg values (that is also a problem with the current
implementation) is that it will not catch resister type mix ups. But agreed,
using the encoded reg value is better.

[Karl, 2016/02/05 17:00:05]

Done.

+}
+
+Variable *InstARM32Pop::getStackSReg(SizeT Index, SizeT Size) const {

[John, 2016/02/05 01:46:25]

Do you need to pass Size here? Isn't size the same as Dest.size()? If so, just
invoke it here

[Karl, 2016/02/05 17:00:05]

No longer needed. Removed need for virtuals.

+  return getStackReg(Size - (Index + 1));
+}
+
+const Variable *InstARM32Pop::updateBaseReg(const Variable *BaseReg,
+                                            const Variable *NextReg) const {
+  (void)BaseReg;
+  return NextReg;
+}
+
 void InstARM32Pop::emitSingleGPR(const Cfg *Func, const EmitForm Form,
                                  const Variable *Reg) const {
   switch (Form) {
   case Emit_Text:
     emitGPRsAsText(Func);
     return;
   case Emit_Binary:
     Func->getAssembler<ARM32::AssemblerARM32>()->pop(Reg, CondARM32::AL);
     return;
   }
@@ skipping 28 matching lines @@
 const char *InstARM32Push::getGPROpcode() const { return "push"; }
 
 const char *InstARM32Push::getSRegOpcode() const { return "vpush"; }
 
 Variable *InstARM32Push::getStackReg(SizeT Index) const {
   return llvm::cast<Variable>(getSrc(Index));
 }
 
 SizeT InstARM32Push::getNumStackRegs() const { return getSrcSize(); }
 
+bool InstARM32Push::isAssignedConsecutiveRegisters(
+    const Variable *Before, const Variable *After) const {
+  return Before->getRegNum() + 1 == After->getRegNum();
+}
+
+Variable *InstARM32Push::getStackSReg(SizeT Index, SizeT Size) const {
+  (void)Size;
+  return getStackReg(Index);
+}
+
+const Variable *InstARM32Push::updateBaseReg(const Variable *BaseReg,
+                                             const Variable *NextReg) const {
+  (void)NextReg;
+  return BaseReg;
+}
+
 void InstARM32Push::emitSingleGPR(const Cfg *Func, const EmitForm Form,
                                   const Variable *Reg) const {
   switch (Form) {
   case Emit_Text:
     emitGPRsAsText(Func);
     return;
   case Emit_Binary:
     Func->getAssembler<ARM32::AssemblerARM32>()->push(Reg, CondARM32::AL);
     return;
   }
@@ skipping 706 matching lines @@
 
 template class InstARM32FourAddrGPR<InstARM32::Mla>;
 template class InstARM32FourAddrGPR<InstARM32::Mls>;
 
 template class InstARM32CmpLike<InstARM32::Cmn>;
 template class InstARM32CmpLike<InstARM32::Cmp>;
 template class InstARM32CmpLike<InstARM32::Tst>;
 
 } // end of namespace ARM32
 } // end of namespace Ice