Subzero. ARM32. Show FP lowering some love.

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 167 matching lines @@
   assert(Inst->getSrcSize() == 2);
   Str << "\t" << Opcode << getVecWidthString(Inst->getDest()->getType())
       << "\t";
   Inst->getDest()->emit(Func);
   Str << ", ";
   Inst->getSrc(0)->emit(Func);
   Str << ", ";
   Inst->getSrc(1)->emit(Func);
 }
 
+void InstARM32::emitFourAddrFP(const char *Opcode, const InstARM32 *Inst,
+                               const Cfg *Func) {
+  if (!BuildDefs::dump())
+    return;
+  Ostream &Str = Func->getContext()->getStrEmit();
+  assert(Inst->getSrcSize() == 3);
+  assert(Inst->getSrc(0) == Inst->getDest());
+  Str << "\t" << Opcode << getVecWidthString(Inst->getDest()->getType())
+      << "\t";
+  Inst->getDest()->emit(Func);
+  Str << ", ";
+  Inst->getSrc(1)->emit(Func);
+  Str << ", ";
+  Inst->getSrc(2)->emit(Func);
+}
+
 void InstARM32Pred::emitFourAddr(const char *Opcode, const InstARM32Pred *Inst,
                                  const Cfg *Func) {
   if (!BuildDefs::dump())
     return;
   Ostream &Str = Func->getContext()->getStrEmit();
   assert(Inst->getSrcSize() == 3);
   Str << "\t" << Opcode << Inst->getPredicate() << "\t";
   Inst->getDest()->emit(Func);
   Str << ", ";
   Inst->getSrc(0)->emit(Func);
@@ skipping 358 matching lines @@
 IceString InstARM32Label::getName(const Cfg *Func) const {
   return ".L" + Func->getFunctionName() + "$local$__" + std::to_string(Number);
 }
 
 InstARM32Pop::InstARM32Pop(Cfg *Func, const VarList &Dests)
     : InstARM32(Func, InstARM32::Pop, 0, nullptr), Dests(Dests) {
   // Track modifications to Dests separately via FakeDefs. Also, a pop
   // instruction affects the stack pointer and so it should not be allowed to
   // be automatically dead-code eliminated. This is automatic since we leave
   // the Dest as nullptr.
+  Type PreviousTy = IceType_void;

[Jim Stichnoth, 2015/12/07 20:58:14]

What do you think about adding this?

  if (BuildDefs::minimal())
    return;

Also, I think the validation for Pop and Push could be factored out into a
separate function in an anonymous namespace, and I think it's OK if the result
is that you ultimately iterate twice over Srcs in the Push ctor.

[John, 2015/12/08 13:54:24]

This seems like a serious error that should be verified regardless of minimal
build, but I could be convinced otherwise. What are the arguments for removing
it?

[Jim Stichnoth, 2015/12/08 19:20:16]

The only argument for removing it would be a very minor performance argument in
MINIMAL mode, plus one fewer string in the binary.

I'm OK either way.

+  for (Variable *Dest : Dests) {
+    if (PreviousTy != IceType_void && Dest->getType() != PreviousTy) {
+      llvm::report_fatal_error("Type mismatch when popping registers.");
+    }
+    PreviousTy = Dest->getType();
+  }
 }
 
 InstARM32Push::InstARM32Push(Cfg *Func, const VarList &Srcs)
     : InstARM32(Func, InstARM32::Push, Srcs.size(), nullptr) {
-  for (Variable *Source : Srcs)
+  Type PreviousTy = IceType_void;
+  for (Variable *Source : Srcs) {
+    if (PreviousTy != IceType_void && Source->getType() != PreviousTy) {
+      llvm::report_fatal_error("Type mismatch when pushing registers.");
+    }
     addSource(Source);
+    PreviousTy = Source->getType();
+  }
 }
 
 InstARM32Ret::InstARM32Ret(Cfg *Func, Variable *LR, Variable *Source)
     : InstARM32(Func, InstARM32::Ret, Source ? 2 : 1, nullptr) {
   addSource(LR);
   if (Source)
     addSource(Source);
 }
 
 InstARM32Str::InstARM32Str(Cfg *Func, Variable *Value, OperandARM32Mem *Mem,
@@ skipping 125 matching lines @@
 template <> const char *InstARM32Orr::Opcode = "orr";
 template <> const char *InstARM32Rsb::Opcode = "rsb";
 template <> const char *InstARM32Rsc::Opcode = "rsc";
 template <> const char *InstARM32Sbc::Opcode = "sbc";
 template <> const char *InstARM32Sdiv::Opcode = "sdiv";
 template <> const char *InstARM32Sub::Opcode = "sub";
 template <> const char *InstARM32Udiv::Opcode = "udiv";
 // FP
 template <> const char *InstARM32Vadd::Opcode = "vadd";
 template <> const char *InstARM32Vdiv::Opcode = "vdiv";
+template <> const char *InstARM32Veor::Opcode = "veor";
+template <> const char *InstARM32Vmla::Opcode = "vmla";
+template <> const char *InstARM32Vmls::Opcode = "vmls";
 template <> const char *InstARM32Vmul::Opcode = "vmul";
-template <> const char *InstARM32Veor::Opcode = "veor";
 template <> const char *InstARM32Vsub::Opcode = "vsub";
 // Four-addr ops
 template <> const char *InstARM32Mla::Opcode = "mla";
 template <> const char *InstARM32Mls::Opcode = "mls";
 // Cmp-like ops
 template <> const char *InstARM32Cmn::Opcode = "cmn";
 template <> const char *InstARM32Cmp::Opcode = "cmp";
 template <> const char *InstARM32Tst::Opcode = "tst";
 
 void InstARM32::dump(const Cfg *Func) const {
@@ skipping 450 matching lines @@
 }
 
 template <> void InstARM32Uxt::emitIAS(const Cfg *Func) const {
   assert(getSrcSize() == 1);
   auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
   Asm->uxt(getDest(), getSrc(0), getPredicate());
   if (Asm->needsTextFixup())
     emitUsingTextFixup(Func);
 }
 
+namespace {
+
+bool AssignedConsecutiveRegisters(Operand *Before, Operand *After) {

[Jim Stichnoth, 2015/12/07 20:58:14]

Maybe isAssignedConsecutiveRegisters() ?

(in any case, don't capitalize a function name, sorry)

[John, 2015/12/08 13:54:24]

Done.

PS: don't be sorry for the code style. :)

+  return llvm::cast<Variable>(Before)->getRegNum() + 1 ==

[Jim Stichnoth, 2015/12/07 20:58:14]

I think you should make Before and After both Variable* type, and do the cast as
necessary in the caller.

Also, shouldn't you also validate that hasReg() is true as part of the test?

[John, 2015/12/08 13:54:24]

I am asserting here because I moved the actual test to the ctor.

+         llvm::cast<Variable>(After)->getRegNum();
+}
+
+} // end of anonymous namespace
+
 void InstARM32Pop::emit(const Cfg *Func) const {
   // TODO(jpp): Improve FP register save/restore.
   if (!BuildDefs::dump())
     return;
-  SizeT IntegerCount = 0;
-  for (const Operand *Op : Dests) {
-    if (isScalarIntegerType(Op->getType())) {
-      ++IntegerCount;
-    }
+  Ostream &Str = Func->getContext()->getStrEmit();

[Jim Stichnoth, 2015/12/07 20:58:14]

Maybe move this after the DestSize assert / early return

[John, 2015/12/08 13:54:24]

Done.

+  const SizeT DestSize = Dests.size();
+  assert(DestSize > 0);

[Jim Stichnoth, 2015/12/07 20:58:14]

Maybe move the assert into the "if" branch, right before the return?  I think
that documents the intention slightly better.

[John, 2015/12/08 13:54:24]

Done.

[Jim Stichnoth, 2015/12/08 19:20:16]

Actually, looks like not (yet) done...

[John, 2015/12/09 13:11:08]

Done.

+  if (DestSize == 0) {
+    return;
   }
-  Ostream &Str = Func->getContext()->getStrEmit();
-  bool NeedNewline = false;
-  if (IntegerCount != 0) {
+
+  Operand *Op = Dests[0];

[Jim Stichnoth, 2015/12/07 20:58:14]

Can this be declared as Variable* ?  Maybe "Variable *Var"?

In any case, I would name it "Opnd" rather than "Op", because the latter is (in
my opinion) ambiguous between "operand" and "operation" and "opcode".

[John, 2015/12/08 13:54:24]

Done.

+  if (isScalarIntegerType(Op->getType())) {
+    // GPR push.
     Str << "\t"
-        << "pop"
-        << "\t{";
-    bool PrintComma = false;
-    for (const Operand *Op : Dests) {
-      if (isScalarIntegerType(Op->getType())) {
-        if (PrintComma)
-          Str << ", ";
-        Op->emit(Func);
-        PrintComma = true;
-      }
+           "pop"
+           "\t{";
+    Op->emit(Func);
+    for (SizeT i = 1; i < DestSize; ++i) {
+      Str << ", ";
+      Op = Dests[i];
+      Op->emit(Func);
     }
     Str << "}";
-    NeedNewline = true;
+    return;

[Karl, 2015/12/07 19:11:07]

I assume you return here because you modified the target lowering to not combine
GP registers (pop) with other registers (vpop)?

[John, 2015/12/08 13:54:24]

That's correct.

   }
 
-  for (const Operand *Op : Dests) {
-    if (isScalarIntegerType(Op->getType()))
-      continue;
-    if (NeedNewline) {
-      Str << "\n";
+  // VFP "s" reg push.
+  SizeT End = DestSize - 1;
+  SizeT Start = DestSize - 1;
+  Op = Dests[DestSize - 1];
+  Str << "\t"
+         "vpop"
+         "\t{";
+  for (SizeT i = 2; i <= DestSize; ++i) {
+    Operand *PreviousOp = Dests[DestSize - i];
+    if (!AssignedConsecutiveRegisters(PreviousOp, Op)) {
+      Dests[Start]->emit(Func);
+      for (SizeT j = Start + 1; j <= End; ++j) {
+        Str << ", ";
+        Dests[j]->emit(Func);
+      }
       startNextInst(Func);
-      NeedNewline = false;
+      Str << "}\n\t"
+             "vpop"
+             "\t{";
+      End = DestSize - i;
     }
-    Str << "\t"
-        << "vpop"
-        << "\t{";
-    Op->emit(Func);
-    Str << "}";
-    NeedNewline = true;
+    Op = PreviousOp;
+    Start = DestSize - i;
   }
-  assert(NeedNewline); // caller will add the newline
+  Dests[Start]->emit(Func);
+  for (SizeT j = Start + 1; j <= End; ++j) {
+    Str << ", ";
+    Dests[j]->emit(Func);
+  }
+  Str << "}";
 }
 
 void InstARM32Pop::emitIAS(const Cfg *Func) const {
   SizeT IntegerCount = 0;
   ARM32::IValueT GPRegisters = 0;
   const Variable *LastDest = nullptr;
   for (const Variable *Var : Dests) {
     if (!isScalarIntegerType(Var->getType()))
       // TODO(kschimpf) Implement vpush.
       return emitUsingTextFixup(Func);
     assert((Var && Var->hasReg()) && "pop only applies to registers");
     int32_t Reg = Var->getRegNum();
     assert(Reg != RegARM32::Encoded_Not_GPR);
     LastDest = Var;
     GPRegisters |= (1 << Reg);
     ++IntegerCount;
   }
   auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
   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!");
-    // TODO(kschimpf) ARM sandbox does not allow the single register form of
+    // TODO(kschimpf): ARM sandbox does not allow the single register form of
     // pop, and the popList form expects multiple registers. Convert this
     // assert to a conditional check once it has been shown that popList
     // works.
     assert(!Func->getContext()->getFlags().getUseSandboxing() &&
            "pop register not in ARM sandbox!");
     Asm->pop(LastDest, CondARM32::AL);
     break;
   default:
     Asm->popList(GPRegisters, CondARM32::AL);
     break;
@@ skipping 12 matching lines @@
     if (I > 0)
       Str << ", ";
     Dests[I]->dump(Func);
   }
 }
 
 void InstARM32Push::emit(const Cfg *Func) const {
   // TODO(jpp): Improve FP register save/restore.
   if (!BuildDefs::dump())
     return;
-  SizeT IntegerCount = 0;
-  for (SizeT i = 0; i < getSrcSize(); ++i) {
-    if (isScalarIntegerType(getSrc(i)->getType())) {
-      ++IntegerCount;
-    }
+  Ostream &Str = Func->getContext()->getStrEmit();
+  const SizeT SrcSize = getSrcSize();
+  assert(SrcSize > 0);
+  if (SrcSize == 0) {
+    return;
   }
-  Ostream &Str = Func->getContext()->getStrEmit();
-  bool NeedNewline = false;
-  for (SizeT i = getSrcSize(); i > 0; --i) {
-    Operand *Op = getSrc(i - 1);
-    if (isScalarIntegerType(Op->getType()))
-      continue;
-    if (NeedNewline) {
-      Str << "\n";
-      startNextInst(Func);
-      NeedNewline = false;
-    }
+
+  Operand *Op = getSrc(0);
+  if (isScalarIntegerType(Op->getType())) {
+    // GPR push.
     Str << "\t"
-        << "vpush"
-        << "\t{";
+           "push"
+           "\t{";
     Op->emit(Func);
-    Str << "}";
-    NeedNewline = true;
-  }
-  if (IntegerCount != 0) {
-    if (NeedNewline) {
-      Str << "\n";
-      startNextInst(Func);
-      NeedNewline = false;
-    }
-    Str << "\t"
-        << "push"
-        << "\t{";
-    bool PrintComma = false;
-    for (SizeT i = 0; i < getSrcSize(); ++i) {
-      Operand *Op = getSrc(i);
-      if (isScalarIntegerType(Op->getType())) {
-        if (PrintComma)
-          Str << ", ";
-        Op->emit(Func);
-        PrintComma = true;
-      }
+    for (SizeT i = 1; i < SrcSize; ++i) {
+      Str << ", ";
+      Op = getSrc(i);
+      Op->emit(Func);
     }
     Str << "}";
-    NeedNewline = true;
+    return;
   }
-  assert(NeedNewline); // caller will add the newline
+
+  // VFP "s" reg push.
+  Str << "\t"
+         "vpush"
+         "\t{";
+  Op->emit(Func);
+  for (SizeT i = 1; i < SrcSize; ++i) {
+    Operand *NextOp = getSrc(i);
+    if (AssignedConsecutiveRegisters(Op, NextOp)) {
+      Str << ", ";
+    } else {
+      startNextInst(Func);
+      Str << "}\n\t"
+             "vpush"
+             "\t{";
+    }
+    Op = NextOp;
+    Op->emit(Func);
+  }
+  Str << "}";
 }
 
 void InstARM32Push::emitIAS(const Cfg *Func) const {
   SizeT IntegerCount = 0;
   ARM32::IValueT GPRegisters = 0;
   const Variable *LastSrc = nullptr;
   for (SizeT Index = 0; Index < getSrcSize(); ++Index) {
     if (!isScalarIntegerType(getSrc(Index)->getType()))
       // TODO(kschimpf) Implement vpush.
       return emitUsingTextFixup(Func);
@@ skipping 536 matching lines @@
 template class InstARM32ThreeAddrGPR<InstARM32::Orr>;
 template class InstARM32ThreeAddrGPR<InstARM32::Rsb>;
 template class InstARM32ThreeAddrGPR<InstARM32::Rsc>;
 template class InstARM32ThreeAddrGPR<InstARM32::Sbc>;
 template class InstARM32ThreeAddrGPR<InstARM32::Sdiv>;
 template class InstARM32ThreeAddrGPR<InstARM32::Sub>;
 template class InstARM32ThreeAddrGPR<InstARM32::Udiv>;
 
 template class InstARM32ThreeAddrFP<InstARM32::Vadd>;
 template class InstARM32ThreeAddrFP<InstARM32::Vdiv>;
+template class InstARM32ThreeAddrFP<InstARM32::Veor>;
 template class InstARM32ThreeAddrFP<InstARM32::Vmul>;
-template class InstARM32ThreeAddrFP<InstARM32::Veor>;
+template class InstARM32ThreeAddrFP<InstARM32::Vmla>;
+template class InstARM32ThreeAddrFP<InstARM32::Vmls>;
 template class InstARM32ThreeAddrFP<InstARM32::Vsub>;
 
 template class InstARM32LoadBase<InstARM32::Ldr>;
 template class InstARM32LoadBase<InstARM32::Ldrex>;
 
 template class InstARM32TwoAddrGPR<InstARM32::Movt>;
 
 template class InstARM32UnaryopGPR<InstARM32::Movw, false>;
 template class InstARM32UnaryopGPR<InstARM32::Clz, false>;
 template class InstARM32UnaryopGPR<InstARM32::Mvn, false>;
 template class InstARM32UnaryopGPR<InstARM32::Rbit, false>;
 template class InstARM32UnaryopGPR<InstARM32::Rev, false>;
 template class InstARM32UnaryopGPR<InstARM32::Sxt, true>;
 template class InstARM32UnaryopGPR<InstARM32::Uxt, true>;
 template class InstARM32UnaryopFP<InstARM32::Vsqrt>;
 
 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 Ice