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

src/IceInstARM32.cpp

@@ -900,13 +900,23 @@ void InstARM32RegisterStackOp::emitSRegsAsText(const Cfg *Func,
   Str << "}";
 }
 
+void InstARM32RegisterStackOp::emitSRegsOp(const Cfg *Func, EmitForm Form,
+                                           const Variable *BaseReg,
+                                           SizeT RegCount,
+                                           SizeT InstIndex) const {
+  if (Form == Emit_Text && BuildDefs::dump() && InstIndex > 0) {
+    startNextInst(Func);
+    Func->getContext()->getStrEmit() << "\n";
+  }
+  emitSRegs(Func, Form, BaseReg, RegCount);
+}
+
 namespace {
 
 bool isAssignedConsecutiveRegisters(const Variable *Before,
                                     const Variable *After) {
-  assert(Before->hasReg());
-  assert(After->hasReg());
-  return Before->getRegNum() + 1 == After->getRegNum();
+  return RegARM32::getEncodedSReg(Before->getRegNum()) + 1 ==
+         RegARM32::getEncodedSReg(After->getRegNum());
 }
 
 } // end of anonymous namespace
@@ -918,7 +928,7 @@ void InstARM32RegisterStackOp::emitUsingForm(const Cfg *Func,
 
   const auto *Reg = llvm::cast<Variable>(getStackReg(0));
   if (isScalarIntegerType(Reg->getType())) {
-    // Pop GPR registers.
+    // Push/pop GPR registers.
     SizeT IntegerCount = 0;
     ARM32::IValueT GPRegisters = 0;
     const Variable *LastDest = nullptr;
@@ -935,33 +945,45 @@ void InstARM32RegisterStackOp::emitUsingForm(const Cfg *Func,
     } 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);
-      if (BaseReg == nullptr) {
-        BaseReg = NextReg;
-        RegCount = 1;
-      } else if (RegCount < VpushVpopMaxConsecRegs &&
-                 isAssignedConsecutiveRegisters(Reg, NextReg)) {
-        ++RegCount;
-      } else {
-        emitSRegs(Func, Form, BaseReg, RegCount);
-        if (Form == Emit_Text && BuildDefs::dump()) {
-          startNextInst(Func);
-          Func->getContext()->getStrEmit() << "\n";
-        }
-        BaseReg = NextReg;
-        RegCount = 1;
-      }
-      Reg = NextReg;
-    }
-    if (RegCount) {
-      emitSRegs(Func, Form, BaseReg, RegCount);
+    return;
+  }
+
+  // Push/pop floating point registers. Divide into a list of instructions,
+  // defined on consecutive register ranges. Then generate the corresponding
+  // instructions.
+
+  // Typical max number of registers ranges pushed/popd is no more than 5.
+  llvm::SmallVector<std::pair<const Variable *, SizeT>, 5> InstData;
+  const Variable *BaseReg = nullptr;
+  SizeT RegCount = 0;
+  for (SizeT i = 0; i < NumRegs; ++i) {
+    const Variable *NextReg = getStackReg(i);
+    assert(NextReg->hasReg());
+    if (BaseReg == nullptr) {
+      BaseReg = NextReg;
+      RegCount = 1;
+    } else if (RegCount < VpushVpopMaxConsecRegs &&
+               isAssignedConsecutiveRegisters(Reg, NextReg)) {
+      ++RegCount;
+    } else {
+      InstData.emplace_back(BaseReg, RegCount);
+      BaseReg = NextReg;
+      RegCount = 1;
     }
+    Reg = NextReg;
+  }
+  if (RegCount) {
+    InstData.emplace_back(BaseReg, RegCount);
+  }
+  SizeT InstCount = 0;
+  if (llvm::isa<InstARM32Push>(*this)) {
+    for (const auto &Pair : InstData)
+      emitSRegsOp(Func, Form, Pair.first, Pair.second, InstCount++);
+    return;
   }
+  assert(llvm::isa<InstARM32Pop>(*this));
+  for (const auto &Pair : reverse_range(InstData))
+    emitSRegsOp(Func, Form, Pair.first, Pair.second, InstCount++);
 }
 
 InstARM32Pop::InstARM32Pop(Cfg *Func, const VarList &Dests)

src/IceInstARM32.h

@@ -1094,6 +1094,8 @@ protected:
   void emitGPRsAsText(const Cfg *Func) const;
   void emitSRegsAsText(const Cfg *Func, const Variable *BaseReg,
                        SizeT Regcount) const;
+  void emitSRegsOp(const Cfg *Func, const EmitForm, const Variable *BaseReg,
+                   SizeT RegCount, SizeT InstIndex) const;
   virtual const char *getDumpOpcode() const { return getGPROpcode(); }
   virtual const char *getGPROpcode() const = 0;
   virtual const char *getSRegOpcode() const = 0;
@@ -1109,7 +1111,7 @@ protected:
 
 /// Pops a list of registers. It may be a list of GPRs, or a list of VFP "s"
 /// regs, but not both. In any case, the list must be sorted.
-class InstARM32Pop : public InstARM32RegisterStackOp {
+class InstARM32Pop final : public InstARM32RegisterStackOp {
   InstARM32Pop() = delete;
   InstARM32Pop(const InstARM32Pop &) = delete;
   InstARM32Pop &operator=(const InstARM32Pop &) = delete;
@@ -1132,13 +1134,12 @@ private:
                         IValueT Registers) const final;
   void emitSRegs(const Cfg *Func, const EmitForm Form, const Variable *BaseReg,
                  SizeT RegCount) const final;
-
   VarList Dests;
 };
 
 /// Pushes a list of registers. Just like Pop (see above), the list may be of
 /// GPRs, or VFP "s" registers, but not both.
-class InstARM32Push : public InstARM32RegisterStackOp {
+class InstARM32Push final : public InstARM32RegisterStackOp {
   InstARM32Push() = delete;
   InstARM32Push(const InstARM32Push &) = delete;
   InstARM32Push &operator=(const InstARM32Push &) = delete;

tests_lit/assembler/arm32/popmult.ll

+; Show that pops are generated in reverse order of pushes.
+
+; NOTE: Restricts to nonconsecutive S registers to force the generation of
+; multiple vpush/vpop instructions. Also tests that we generate them in the
+; right order (the reverse of the corresponding push). Uses -O2 to keep all
+; results in S registers.
+
+; REQUIRES: allow_dump
+
+; Compile using standalone assembler.
+; RUN: %p2i --filetype=asm -i %s --target=arm32 --args -O2 -allow-extern \
+; RUN:   -reg-use s20,s22,s23 \
+; RUN:   | FileCheck %s --check-prefix=ASM
+
+; Show bytes in assembled standalone code.
+; RUN: %p2i --filetype=asm -i %s --target=arm32 --assemble --disassemble \
+; RUN:   --args -O2 -allow-extern \
+; RUN:   -reg-use s20,s22,s23 \
+; RUN:   | FileCheck %s --check-prefix=DIS
+
+; Compile using integrated assembler.
+; RUN: %p2i --filetype=iasm -i %s --target=arm32 --args -O2 -allow-extern \
+; RUN:   -reg-use s20,s22,s23 \
+; RUN:   | FileCheck %s --check-prefix=IASM
+
+; Show bytes in assembled integrated code.
+; RUN: %p2i --filetype=iasm -i %s --target=arm32 --assemble --disassemble \
+; RUN:   --args -O2 -allow-extern \
+; RUN:   -reg-use s20,s22,s23 \
+; RUN:   | FileCheck %s --check-prefix=DIS
+
+declare external void @f()
+
+define internal float @test2SPops(float %p1, float %p2) {
+; ASM-LABEL: test2SPops:
+; DIS-LABEL: 00000000 <test2SPops>:
+; IASM-LABEL: test2SPops:
+
+; ASM:          vpush   {s20}
+; ASM-NEXT:     vpush   {s22, s23}
+; ASM-NEXT:     push    {lr}
+
+; DIS:          0:      ed2daa01
+; DIS-NEXT:     4:      ed2dba02
+; DIS-NEXT:     8:      e52de004
+
+; IASM-NOT:     vpush
+; IASM-NOT:     push
+
+  %v1 = fadd float %p1, %p2
+  %v2 = fsub float %p1, %p2
+  %v3 = fsub float %p2, %p1
+  call void @f()
+  %v4 = fadd float %v1, %v2
+  %res = fadd float %v3, %v4
+
+; ASM:          pop     {lr}
+; ASM-NEXT:     # lr = def.pseudo
+; ASM-NEXT:     vpop    {s22, s23}
+; ASM-NEXT:     vpop    {s20}
+
+; DIS:         40:      e49de004
+; DIS-NEXT:    44:      ecbdba02
+; DIS-NEXT:    48:      ecbdaa01
+
+; IASM-NOT: pop
+; IASM-NOT: vpop
+
+  ret float %res
+}