Clean up handling of ARM IR instruction "mov".

src/IceInstARM32.cpp

Index: src/IceInstARM32.cpp
diff --git a/src/IceInstARM32.cpp b/src/IceInstARM32.cpp
index a1605828e3eae08637d8615c6632cd633303a5de..5e1ebd2105ce5bb091f9fa299fc2e13eab810054 100644
--- a/src/IceInstARM32.cpp
+++ b/src/IceInstARM32.cpp
@@ -1015,31 +1015,38 @@ void InstARM32::dump(const Cfg *Func) const {
   Inst::dump(Func);
 }
 
-void InstARM32Mov::emitMultiDestSingleSource(const Cfg *Func) const {
-  if (!BuildDefs::dump())
-    return;
-  Ostream &Str = Func->getContext()->getStrEmit();
+void InstARM32Mov::emitMultiDestSingleSource(const Cfg *Func,
+                                             const EmitForm Form) const {

[John, 2016/01/16 00:24:13]

I understand what you're doing, but it seems to me that this change makes things
harder to follow. Every method (with the exception of emiUsingForm) is
essentially initializing variables, then going about to do completely different
things for the different output types.

I believe it would be better if, instead of merging the methods, you created
helper functions with common logic (eg, IsVmove), and called them in separate
functions for textual, and binary outputs.

This cl also increases the minimum build, for the compiler will not be able to
DCE the prolog to these functions.

[Karl, 2016/01/19 18:32:39]

Ok. For the moment, I will not try to merge code between the text and binary
forms. Refactoring to match this change.

   Variable *DestLo = getDest();
   Variable *DestHi = getDestHi();
-  auto *Src = llvm::cast<Variable>(getSrc(0));
-
   assert(DestHi->hasReg());
   assert(DestLo->hasReg());
+  auto *Src = llvm::cast<Variable>(getSrc(0));
   assert(llvm::isa<Variable>(Src) && Src->hasReg());
-
-  Str << "\t"
-         "vmov" << getPredicate() << "\t";
-  DestLo->emit(Func);
-  Str << ", ";
-  DestHi->emit(Func);
-  Str << ", ";
-  Src->emit(Func);
+  switch (Form) {
+  case Emit_Text: {
+    if (!BuildDefs::dump())
+      return;
+    Ostream &Str = Func->getContext()->getStrEmit();
+    Str << "\t"
+           "vmov" << getPredicate() << "\t";
+    DestLo->emit(Func);
+    Str << ", ";
+    DestHi->emit(Func);
+    Str << ", ";
+    Src->emit(Func);
+    return;
+  }
+  case Emit_Binary: {
+    auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
+    Asm->setNeedsTextFixup();
+    return;
+  }
+  }
 }
 
-void InstARM32Mov::emitSingleDestMultiSource(const Cfg *Func) const {
-  if (!BuildDefs::dump())
-    return;
-  Ostream &Str = Func->getContext()->getStrEmit();
+void InstARM32Mov::emitSingleDestMultiSource(const Cfg *Func,
+                                             const EmitForm Form) const {
   Variable *Dest = getDest();
   auto *SrcLo = llvm::cast<Variable>(getSrc(0));
   auto *SrcHi = llvm::cast<Variable>(getSrc(1));
@@ -1049,13 +1056,26 @@ void InstARM32Mov::emitSingleDestMultiSource(const Cfg *Func) const {
   assert(Dest->hasReg());
   assert(getSrcSize() == 2);
 
-  Str << "\t"
-         "vmov" << getPredicate() << "\t";
-  Dest->emit(Func);
-  Str << ", ";
-  SrcLo->emit(Func);
-  Str << ", ";
-  SrcHi->emit(Func);
+  switch (Form) {
+  case Emit_Text: {
+    if (!BuildDefs::dump())
+      return;
+    Ostream &Str = Func->getContext()->getStrEmit();
+    Str << "\t"
+           "vmov" << getPredicate() << "\t";
+    Dest->emit(Func);
+    Str << ", ";
+    SrcLo->emit(Func);
+    Str << ", ";
+    SrcHi->emit(Func);
+    return;
+  }
+  case Emit_Binary: {
+    auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
+    Asm->setNeedsTextFixup();
+    return;
+  }
+  }
 }
 
 namespace {
@@ -1079,12 +1099,9 @@ bool isMoveBetweenCoreAndVFPRegisters(Variable *Dest, Operand *Src) {
 
 } // end of anonymous namespace
 
-void InstARM32Mov::emitSingleDestSingleSource(const Cfg *Func) const {
-  if (!BuildDefs::dump())
-    return;
-  Ostream &Str = Func->getContext()->getStrEmit();
+void InstARM32Mov::emitSingleDestSingleSource(const Cfg *Func,
+                                              const EmitForm Form) const {
   Variable *Dest = getDest();
-
   if (!Dest->hasReg()) {
     llvm::report_fatal_error("mov can't store.");
   }
@@ -1094,75 +1111,97 @@ void InstARM32Mov::emitSingleDestSingleSource(const Cfg *Func) const {
     llvm::report_fatal_error("mov can't load.");
   }
 
-  Type Ty = Dest->getType();
-  const bool IsVector = isVectorType(Ty);
-  const bool IsScalarFP = isScalarFloatingType(Ty);
+  Type DestTy = Dest->getType();
+  const bool IsVector = isVectorType(DestTy);
+  const bool IsScalarFP = isScalarFloatingType(DestTy);
   const bool CoreVFPMove = isMoveBetweenCoreAndVFPRegisters(Dest, Src0);
   const bool IsVMove = (IsVector || IsScalarFP || CoreVFPMove);
-  const char *Opcode = IsVMove ? "vmov" : "mov";
-  // when vmov{c}'ing, we need to emit a width string. Otherwise, the
-  // assembler might be tempted to assume we want a vector vmov{c}, and that
-  // is disallowed because ARM.
-  const char *WidthString = !CoreVFPMove ? getVecWidthString(Ty) : "";
-  Str << "\t" << Opcode;
   if (IsVMove) {
-    Str << getPredicate() << WidthString;
-  } else {
-    Str << WidthString << getPredicate();
+    switch (Form) {
+    case Emit_Text: {
+      if (!BuildDefs::dump())
+        return;
+      Ostream &Str = Func->getContext()->getStrEmit();
+      Variable *Dest = getDest();
+      Operand *Src0 = getSrc(0);
+      // when vmov{c}'ing, we need to emit a width string. Otherwise, the
+      // assembler might be tempted to assume we want a vector vmov{c}, and
+      // that is disallowed.
+      const bool CoreVFPMove = isMoveBetweenCoreAndVFPRegisters(Dest, Src0);
+      const char *WidthString = !CoreVFPMove ? getVecWidthString(DestTy) : "";
+      Str << "\t"
+          << "vmov" << getPredicate() << WidthString << "\t";
+      Dest->emit(Func);
+      Str << ", ";
+      Src0->emit(Func);
+      return;
+    }
+    case Emit_Binary: {
+      auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
+      if (!llvm::isa<Variable>(Src0))
+        // TODO(kschimpf) Handle moving contants into registers.
+        return Asm->setNeedsTextFixup();
+
+      // Move register to register.
+      switch (DestTy) {
+      default:
+        // TODO(kschimpf): Fill this out more.
+        return Asm->setNeedsTextFixup();
+      case IceType_f32:
+        Type Src0Ty = Src0->getType();
+        switch (Src0Ty) {
+        default:
+          // TODO(kschimpf): Fill this out more.
+          return Asm->setNeedsTextFixup();
+        case IceType_i32:
+          return Asm->vmovsr(Dest, Src0, getPredicate());
+        }
+      }
+    }
+    }
   }
-  Str << "\t";
-  Dest->emit(Func);
-  Str << ", ";
-  Src0->emit(Func);
-}
 
-void InstARM32Mov::emitIASSingleDestSingleSource(const Cfg *Func) const {
-  auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
-  Variable *Dest = getDest();
-  Operand *Src0 = getSrc(0);
-
-  if (!Dest->hasReg()) {
-    llvm::report_fatal_error("mov can't store.");
+  switch (Form) {
+  case Emit_Text: {
+    Ostream &Str = Func->getContext()->getStrEmit();
+    Str << "\t"
+        << "mov" << getPredicate() << "\t";
+    getDest()->emit(Func);
+    Str << ", ";
+    getSrc(0)->emit(Func);
+    return;
+  }
+  case Emit_Binary: {
+    auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
+    Asm->mov(getDest(), getSrc(0), getPredicate());
+    return;
   }
-
-  if (isMemoryAccess(Src0)) {
-    llvm::report_fatal_error("mov can't load.");
   }
-
-  const Type DestTy = Dest->getType();
-  const bool DestIsVector = isVectorType(DestTy);
-  const bool DestIsScalarFP = isScalarFloatingType(DestTy);
-  const bool CoreVFPMove = isMoveBetweenCoreAndVFPRegisters(Dest, Src0);
-  if (DestIsVector || DestIsScalarFP || CoreVFPMove)
-    return Asm->setNeedsTextFixup();
-  return Asm->mov(Dest, Src0, getPredicate());
 }
 
-void InstARM32Mov::emit(const Cfg *Func) const {
-  if (!BuildDefs::dump())
-    return;
+void InstARM32Mov::emitUsingForm(const Cfg *Func, const EmitForm Form) const {
   assert(!(isMultiDest() && isMultiSource()) && "Invalid vmov type.");
   if (isMultiDest()) {
-    emitMultiDestSingleSource(Func);
+    emitMultiDestSingleSource(Func, Form);
     return;
   }
 
   if (isMultiSource()) {
-    emitSingleDestMultiSource(Func);
+    emitSingleDestMultiSource(Func, Form);
     return;
   }
+  emitSingleDestSingleSource(Func, Form);
+}
 
-  emitSingleDestSingleSource(Func);
+void InstARM32Mov::emit(const Cfg *Func) const {
+  if (!BuildDefs::dump())
+    return;
+  emitUsingForm(Func, Emit_Text);
 }
 
 void InstARM32Mov::emitIAS(const Cfg *Func) const {
-  assert(!(isMultiDest() && isMultiSource()) && "Invalid vmov type.");
+  emitUsingForm(Func, Emit_Binary);
   auto *Asm = Func->getAssembler<ARM32::AssemblerARM32>();
-  if (!(isMultiDest() || isMultiSource()))
-    // Must be single source/dest.
-    emitIASSingleDestSingleSource(Func);
-  else
-    Asm->setNeedsTextFixup();
   if (Asm->needsTextFixup())
     emitUsingTextFixup(Func);
 }