Refactor PUSH/POP in ARM assemblers.

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 70 matching lines @@
 
 void InstARM32Pred::dumpOpcodePred(Ostream &Str, const char *Opcode,
                                    Type Ty) const {
   Str << Opcode << getPredicate() << "." << Ty;
 }
 
 CondARM32::Cond InstARM32::getOppositeCondition(CondARM32::Cond Cond) {
   return InstARM32CondAttributes[Cond].Opposite;
 }
 
+template <>
+void InstARM32::startNextAsmInst<InstARM32::TextualOutput>(
+    const Cfg *Func) const {
+  startNextInst(Func);
+  Func->getContext()->getStrEmit() << "\n";
+}
+
+template <>
+void InstARM32::startNextAsmInst<InstARM32::BinaryOutput>(

[John, 2016/01/06 16:10:21]

The fact that this does nothing tells me this method is not really a template.
You could simply leave as a non-templated method, and only invoke it before
returning on

void InstARM32Pop::emitPop<InstARM32::TextualOutput>()
void InstARM32Pop::emitPopList<InstARM32::TextualOutput>()
void InstARM32Push::emitPush<InstARM32::BinaryOutput>()
void InstARM32Push::emitPushList<InstARM32::TextualOutput>()

[Karl, 2016/01/06 23:21:58]

Removed.

+    const Cfg *Func) const {
+  (void)Func;
+}
+
 void InstARM32::startNextInst(const Cfg *Func) const {
   if (auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>())
     Asm->incEmitTextSize(InstSize);
 }
 
 void InstARM32::emitUsingTextFixup(const Cfg *Func) const {
   if (!BuildDefs::dump())
     return;
   GlobalContext *Ctx = Func->getContext();
   if (Ctx->getFlags().getDisableHybridAssembly()) {
@@ skipping 1216 matching lines @@
 
 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 InstARM32Pop::emit(const Cfg *Func) const {
+void InstARM32Pop::emitPop(const Cfg *Func) const {
   if (!BuildDefs::dump())
     return;
-
+  Ostream &Str = Func->getContext()->getStrEmit();
+  Str << "\t"
+      << "pop"
+      << "\t{";
+  Dests[0]->emit(Func);
   const SizeT DestSize = Dests.size();
-  if (DestSize == 0) {
-    assert(false && "Empty pop list");
-    return;
-  }
-
-  Ostream &Str = Func->getContext()->getStrEmit();
-
-  Variable *Reg = Dests[0];
-  if (isScalarIntegerType(Reg->getType())) {
-    // GPR push.
-    Str << "\t"
-           "pop"
-           "\t{";
-    Reg->emit(Func);
-    for (SizeT i = 1; i < DestSize; ++i) {
-      Str << ", ";
-      Reg = Dests[i];
-      Reg->emit(Func);
-    }
-    Str << "}";
-    return;
-  }
-
-  // VFP "s" reg push.
-  SizeT End = DestSize - 1;
-  SizeT Start = DestSize - 1;
-  Reg = Dests[DestSize - 1];
-  Str << "\t"
-         "vpop"
-         "\t{";
-  for (SizeT i = 2; i <= DestSize; ++i) {
-    Variable *PreviousReg = Dests[DestSize - i];
-    if (!isAssignedConsecutiveRegisters(PreviousReg, Reg)) {
-      Dests[Start]->emit(Func);
-      for (SizeT j = Start + 1; j <= End; ++j) {
-        Str << ", ";
-        Dests[j]->emit(Func);
-      }
-      startNextInst(Func);
-      Str << "}\n\t"
-             "vpop"
-             "\t{";
-      End = DestSize - i;
-    }
-    Reg = PreviousReg;
-    Start = DestSize - i;
-  }
-  Dests[Start]->emit(Func);
-  for (SizeT j = Start + 1; j <= End; ++j) {
+  for (SizeT i = 1; i < DestSize; ++i) {
     Str << ", ";
-    Dests[j]->emit(Func);
+    Dests[i]->emit(Func);
   }
   Str << "}";
 }
 
-void InstARM32Pop::emitIAS(const Cfg *Func) const {
+template <>
+void InstARM32Pop::emitPop<InstARM32::TextualOutput>(
+    const Cfg *Func, const Variable *Reg) const {
+  (void)Reg;
+  this->emitPop(Func);
+}
+
+template <>
+void InstARM32Pop::emitPop<InstARM32::BinaryOutput>(const Cfg *Func,
+                                                    const Variable *Reg) const {
+  auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
+  Asm->pop(Reg, CondARM32::AL);
+}
+
+template <>
+void InstARM32Pop::emitPopList<InstARM32::TextualOutput>(
+    const Cfg *Func, ARM32::IValueT Registers) const {
+  (void)Registers;
+  emitPop(Func);
+}
+
+template <>
+void InstARM32Pop::emitPopList<InstARM32::BinaryOutput>(
+    const Cfg *Func, ARM32::IValueT Registers) const {
+  auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
+  Asm->popList(Registers, CondARM32::AL);
+}
+
+template <>
+void InstARM32Pop::emitVpop<InstARM32::TextualOutput>(const Cfg *Func,
+                                                      const Variable *BaseReg,
+                                                      SizeT RegCount) const {
+  if (!BuildDefs::dump())
+    return;
+  Ostream &Str = Func->getContext()->getStrEmit();
+  Str << "\t"
+      << "vpop"
+      << "\t{";
+  bool IsFirst = true;
+  IValueT Base = AssemblerARM32::getEncodedSRegNum(BaseReg);

[John, 2016/01/06 16:10:21]

Why using this method? RegARM32::getEncodedSReg() returns the same thing, with
the advantage that you don't have to move private, assembler-only routines to
the public interface.

[Karl, 2016/01/06 23:21:58]

Fixed.

+  for (SizeT i = 0; i < RegCount; ++i) {
+    if (IsFirst)
+      IsFirst = false;
+    else
+      Str << ", ";
+    Str << RegARM32::getSRegName(Base + i);
+  }
+  Str << "}";
+}
+
+template <>
+void InstARM32Pop::emitVpop<InstARM32::BinaryOutput>(const Cfg *Func,
+                                                     const Variable *BaseReg,
+                                                     SizeT RegCount) const {
+  auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
+  Asm->vpop(BaseReg, RegCount, CondARM32::AL);
+}
+
+template <InstARM32::OutputForm Form>
+void InstARM32Pop::assemble(const Cfg *Func) const {
+  if (!BuildDefs::dump() && Form == TextualOutput)
+    return;
   // Pop can't be emitted if there are no registers to load. This should never
   // happen, but if it does, we don't need to bring Subzero down -- we just skip
   // emitting the pop instruction (and maybe emit a nop?) The assert() is here
   // so that we can detect this error during development.
   const SizeT DestSize = Dests.size();
   if (DestSize == 0) {
     assert(false && "Empty pop list");
     return;
   }
 
-  auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
   const auto *Reg = llvm::cast<Variable>(Dests[0]);
   if (isScalarIntegerType(Reg->getType())) {
     // Pop GPR registers.
     SizeT IntegerCount = 0;
     ARM32::IValueT GPRegisters = 0;
     const Variable *LastDest = nullptr;
     for (const Variable *Var : Dests) {
       assert(Var->hasReg() && "pop only applies to registers");
       int32_t Reg = RegARM32::getEncodedGPR(Var->getRegNum());
       LastDest = Var;
       GPRegisters |= (1 << Reg);
       ++IntegerCount;
     }
     switch (IntegerCount) {
     case 0:
       return;
     case 1:
       // Note: Can only apply pop register if single register is not sp.
       assert((RegARM32::Encoded_Reg_sp != LastDest->getRegNum()) &&
              "Effects of pop register SP is undefined!");
-      Asm->pop(LastDest, CondARM32::AL);
+      emitPop<Form>(Func, LastDest);
       break;
     default:
-      Asm->popList(GPRegisters, CondARM32::AL);
+      emitPopList<Form>(Func, GPRegisters);
       break;
     }
   } else {
     // Pop vector/floating point registers.
     const Variable *BaseReg = nullptr;
     SizeT RegCount = 0;
     for (const Variable *NextReg : Dests) {
       if (BaseReg == nullptr) {
         BaseReg = NextReg;
         RegCount = 1;
       } else if (RegCount < VpushVpopMaxConsecRegs &&
                  isAssignedConsecutiveRegisters(Reg, NextReg)) {
         ++RegCount;
       } else {
-        Asm->vpop(BaseReg, RegCount, CondARM32::AL);
+        emitVpop<Form>(Func, BaseReg, RegCount);
+        this->startNextAsmInst<Form>(Func);
         BaseReg = NextReg;
         RegCount = 1;
       }
       Reg = NextReg;
     }
-    if (RegCount)
-      Asm->vpop(BaseReg, RegCount, CondARM32::AL);
+    if (RegCount) {
+      emitVpop<Form>(Func, BaseReg, RegCount);
+    }
   }
-  if (Asm->needsTextFixup())
-    emitUsingTextFixup(Func);
+  assert(!Func->getAssembler<ARM32::AssemblerARM32>()->needsTextFixup());
 }
 
 void InstARM32Pop::dump(const Cfg *Func) const {
   if (!BuildDefs::dump())
     return;
   Ostream &Str = Func->getContext()->getStrDump();
   Str << "pop"
       << " ";
   for (SizeT I = 0; I < Dests.size(); ++I) {
     if (I > 0)
       Str << ", ";
     Dests[I]->dump(Func);
   }
 }
 
-void InstARM32Push::emit(const Cfg *Func) const {
+void InstARM32Pop::emit(const Cfg *Func) const {
+  assemble<TextualOutput>(Func);

[John, 2016/01/06 16:10:21]

I would rather have the

if (!BuildDefs::dump()) {
  return;
}

here. Otherwise, I have to look at assemble<TextualOutput>() to realize this
method will not expand in minimal build.

[Karl, 2016/01/06 23:21:58]

Done.

+}
+
+void InstARM32Pop::emitIAS(const Cfg *Func) const {
+  assemble<BinaryOutput>(Func);
+}
+
+void InstARM32Push::emitPush(const Cfg *Func) const {
   if (!BuildDefs::dump())
     return;
-
-  // Push can't be emitted if there are no registers to save. This should never
-  // happen, but if it does, we don't need to bring Subzero down -- we just skip
-  // emitting the push instruction (and maybe emit a nop?) The assert() is here
-  // so that we can detect this error during development.
+  Ostream &Str = Func->getContext()->getStrEmit();
+  Str << "\t"
+      << "push"
+      << "\t{";
+  getSrc(0)->emit(Func);
   const SizeT SrcSize = getSrcSize();
-  if (SrcSize == 0) {
-    assert(false && "Empty push list");
-    return;
-  }
-
-  Ostream &Str = Func->getContext()->getStrEmit();
-
-  const auto *Reg = llvm::cast<Variable>(getSrc(0));
-  if (isScalarIntegerType(Reg->getType())) {
-    // GPR push.
-    Str << "\t"
-           "push"
-           "\t{";
-    Reg->emit(Func);
-    for (SizeT i = 1; i < SrcSize; ++i) {
-      Str << ", ";
-      getSrc(i)->emit(Func);
-    }
-    Str << "}";
-    return;
-  }
-
-  // VFP "s" reg push.
-  Str << "\t"
-         "vpush"
-         "\t{";
-  Reg->emit(Func);
-  SizeT RegCount = 1;
   for (SizeT i = 1; i < SrcSize; ++i) {
-    const auto *NextReg = llvm::cast<Variable>(getSrc(i));
-    if (RegCount < VpushVpopMaxConsecRegs &&
-        isAssignedConsecutiveRegisters(Reg, NextReg)) {
-      ++RegCount;
-      Str << ", ";
-    } else {
-      startNextInst(Func);
-      RegCount = 1;
-      Str << "}\n\t"
-             "vpush"
-             "\t{";
-    }
-    Reg = NextReg;
-    Reg->emit(Func);
+    Str << ", ";
+    getSrc(i)->emit(Func);
   }
   Str << "}";
 }
 
-void InstARM32Push::emitIAS(const Cfg *Func) const {
+template <>
+void InstARM32Push::emitPush<InstARM32::TextualOutput>(
+    const Cfg *Func, const Variable *Reg) const {
+  (void)Reg;
+  this->emitPush(Func);

[John, 2016/01/06 16:10:21]

do you need the

this->

?

[Karl, 2016/01/06 23:21:58]

Removed.

+}
+
+template <>
+void InstARM32Push::emitPush<InstARM32::BinaryOutput>(
+    const Cfg *Func, const Variable *Reg) const {
+  auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
+  Asm->push(Reg, CondARM32::AL);
+}
+
+template <>
+void InstARM32Push::emitPushList<InstARM32::TextualOutput>(
+    const Cfg *Func, ARM32::IValueT Registers) const {
+  (void)Registers;
+  emitPush(Func);
+}
+
+template <>
+void InstARM32Push::emitPushList<InstARM32::BinaryOutput>(
+    const Cfg *Func, ARM32::IValueT Registers) const {
+  auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
+  Asm->pushList(Registers, CondARM32::AL);
+}
+
+template <>
+void InstARM32Push::emitVPush<InstARM32::TextualOutput>(const Cfg *Func,
+                                                        const Variable *BaseReg,
+                                                        SizeT RegCount) const {
+  if (!BuildDefs::dump())
+    return;
+  Ostream &Str = Func->getContext()->getStrEmit();
+  Str << "\t"
+      << "vpush"
+      << "\t{";
+  bool IsFirst = true;
+  IValueT Base = AssemblerARM32::getEncodedSRegNum(BaseReg);

[John, 2016/01/06 16:10:21]

ditto.

[Karl, 2016/01/06 23:21:58]

Done.

+  for (SizeT i = 0; i < RegCount; ++i) {
+    if (IsFirst)
+      IsFirst = false;
+    else
+      Str << ", ";
+    Str << RegARM32::getSRegName(Base + i);
+  }
+  Str << "}";
+}
+
+template <>
+void InstARM32Push::emitVPush<InstARM32::BinaryOutput>(const Cfg *Func,
+                                                       const Variable *BaseReg,
+                                                       SizeT RegCount) const {
+  auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
+  Asm->vpush(BaseReg, RegCount, CondARM32::AL);
+}
+
+template <InstARM32::OutputForm Form>
+void InstARM32Push::assemble(const Cfg *Func) const {

[John, 2016/01/06 16:10:21]

assemble is not really meaningful for textual output. how about naming this
emitCommon?

[Karl, 2016/01/06 23:21:58]

renamed to emitUsingForm().

+  if (!BuildDefs::dump() && Form == TextualOutput)
+    return;
   // Push can't be emitted if there are no registers to save. This should never
   // happen, but if it does, we don't need to bring Subzero down -- we just skip
   // emitting the push instruction (and maybe emit a nop?) The assert() is here
   // so that we can detect this error during development.
   const SizeT SrcSize = getSrcSize();
   if (SrcSize == 0) {
     assert(false && "Empty push list");
     return;
   }
 
-  auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
   const auto *Reg = llvm::cast<Variable>(getSrc(0));
   if (isScalarIntegerType(Reg->getType())) {
     // Push GPR registers.
     SizeT IntegerCount = 0;
     ARM32::IValueT GPRegisters = 0;
     const Variable *LastSrc = nullptr;
     for (SizeT Index = 0; Index < getSrcSize(); ++Index) {
       const auto *Var = llvm::cast<Variable>(getSrc(Index));
       int32_t Reg = RegARM32::getEncodedGPR(Var->getRegNum());
       assert(Reg != RegARM32::Encoded_Not_GPR);
       LastSrc = Var;
       GPRegisters |= (1 << Reg);
       ++IntegerCount;
     }
     switch (IntegerCount) {
     case 0:
       return;

[John, 2016/01/06 16:10:21]

You should llvm::report_fatal_error() here. It is known that there's at least 1
source, and that that source is an integer Variable. This code path should
really never be reached.

[Karl, 2016/01/06 23:21:58]

Actually, this case can't happen due to the test "if (SrcSize == 0)", so
removing.

     case 1: {
       // Note: Can only apply push register if single register is not sp.
       assert((RegARM32::Encoded_Reg_sp != LastSrc->getRegNum()) &&
              "Effects of push register SP is undefined!");
-      Asm->push(LastSrc, CondARM32::AL);
+      emitPush<Form>(Func, LastSrc);
       break;
     }
     default:
-      Asm->pushList(GPRegisters, CondARM32::AL);
+      emitPushList<Form>(Func, GPRegisters);
       break;
     }
   } else {
     // Push vector/Floating point registers.
     const Variable *BaseReg = Reg;
     SizeT RegCount = 1;
     for (SizeT i = 1; i < SrcSize; ++i) {
       const auto *NextReg = llvm::cast<Variable>(getSrc(i));
       if (RegCount < VpushVpopMaxConsecRegs &&
           isAssignedConsecutiveRegisters(Reg, NextReg)) {
         ++RegCount;
       } else {
-        Asm->vpush(BaseReg, RegCount, CondARM32::AL);
+        emitVPush<Form>(Func, BaseReg, RegCount);
+        this->startNextAsmInst<Form>(Func);
         BaseReg = NextReg;
         RegCount = 1;
       }
       Reg = NextReg;
     }
-    Asm->vpush(BaseReg, RegCount, CondARM32::AL);
+    emitVPush<Form>(Func, BaseReg, RegCount);
   }
-  if (Asm->needsTextFixup())
-    emitUsingTextFixup(Func);
+  assert(!Func->getAssembler<ARM32::AssemblerARM32>()->needsTextFixup());
+}
+
+void InstARM32Push::emit(const Cfg *Func) const {
+  assemble<TextualOutput>(Func);
+}
+
+void InstARM32Push::emitIAS(const Cfg *Func) const {
+  assemble<BinaryOutput>(Func);
 }
 
 void InstARM32Push::dump(const Cfg *Func) const {
   if (!BuildDefs::dump())
     return;
   Ostream &Str = Func->getContext()->getStrDump();
   Str << "push"
       << " ";
   dumpSources(Func);
 }
@@ skipping 558 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