Subzero. ARM32. Show FP lowering some love.

src/IceInstARM32.cpp

Index: src/IceInstARM32.cpp
diff --git a/src/IceInstARM32.cpp b/src/IceInstARM32.cpp
index 6932d2f9d01ba502c906ec2e7425c2b60aaeda5a..252c4cc8a10e68a66e4254fae26d37731783fc78 100644
--- a/src/IceInstARM32.cpp
+++ b/src/IceInstARM32.cpp
@@ -185,6 +185,22 @@ void InstARM32::emitThreeAddrFP(const char *Opcode, const InstARM32 *Inst,
   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())
@@ -563,12 +579,25 @@ InstARM32Pop::InstARM32Pop(Cfg *Func, const VarList &Dests)
   // 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)
@@ -714,8 +743,10 @@ template <> const char *InstARM32Udiv::Opcode = "udiv";
 // FP
 template <> const char *InstARM32Vadd::Opcode = "vadd";
 template <> const char *InstARM32Vdiv::Opcode = "vdiv";
-template <> const char *InstARM32Vmul::Opcode = "vmul";
 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 *InstARM32Vsub::Opcode = "vsub";
 // Four-addr ops
 template <> const char *InstARM32Mla::Opcode = "mla";
@@ -1186,51 +1217,72 @@ template <> void InstARM32Uxt::emitIAS(const Cfg *Func) const {
     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.

-  bool NeedNewline = false;
-  if (IntegerCount != 0) {
+  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;
+  }
+
+  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;
+  }
+  Dests[Start]->emit(Func);
+  for (SizeT j = Start + 1; j <= End; ++j) {
+    Str << ", ";
+    Dests[j]->emit(Func);
   }
-  assert(NeedNewline); // caller will add the newline
+  Str << "}";
 }
 
 void InstARM32Pop::emitIAS(const Cfg *Func) const {
@@ -1256,7 +1308,7 @@ void InstARM32Pop::emitIAS(const Cfg *Func) const {
     // 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.
@@ -1289,53 +1341,48 @@ 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();
-  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;
-    }
+  const SizeT SrcSize = getSrcSize();
+  assert(SrcSize > 0);
+  if (SrcSize == 0) {
+    return;
+  }
+
+  Operand *Op = getSrc(0);
+  if (isScalarIntegerType(Op->getType())) {
+    // GPR push.
     Str << "\t"
-        << "vpush"
-        << "\t{";
+           "push"
+           "\t{";
     Op->emit(Func);
+    for (SizeT i = 1; i < SrcSize; ++i) {
+      Str << ", ";
+      Op = getSrc(i);
+      Op->emit(Func);
+    }
     Str << "}";
-    NeedNewline = true;
+    return;
   }
-  if (IntegerCount != 0) {
-    if (NeedNewline) {
-      Str << "\n";
+
+  // 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);
-      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;
-      }
+      Str << "}\n\t"
+             "vpush"
+             "\t{";
     }
-    Str << "}";
-    NeedNewline = true;
+    Op = NextOp;
+    Op->emit(Func);
   }
-  assert(NeedNewline); // caller will add the newline
+  Str << "}";
 }
 
 void InstARM32Push::emitIAS(const Cfg *Func) const {
@@ -1892,8 +1939,10 @@ template class InstARM32ThreeAddrGPR<InstARM32::Udiv>;
 
 template class InstARM32ThreeAddrFP<InstARM32::Vadd>;
 template class InstARM32ThreeAddrFP<InstARM32::Vdiv>;
-template class InstARM32ThreeAddrFP<InstARM32::Vmul>;
 template class InstARM32ThreeAddrFP<InstARM32::Veor>;
+template class InstARM32ThreeAddrFP<InstARM32::Vmul>;
+template class InstARM32ThreeAddrFP<InstARM32::Vmla>;
+template class InstARM32ThreeAddrFP<InstARM32::Vmls>;
 template class InstARM32ThreeAddrFP<InstARM32::Vsub>;
 
 template class InstARM32LoadBase<InstARM32::Ldr>;