Subzero, MIPS32: Extend InstMIPS32Mov to support different data types

src/IceInstMIPS32.cpp

Index: src/IceInstMIPS32.cpp
diff --git a/src/IceInstMIPS32.cpp b/src/IceInstMIPS32.cpp
index efae86753f22cfaff20b644be1be0640ea180edd..7e93dec24d35b2da67021c43d4c47fd19fb611f5 100644
--- a/src/IceInstMIPS32.cpp
+++ b/src/IceInstMIPS32.cpp
@@ -553,29 +553,138 @@ void InstMIPS32Mov::emitSingleDestSingleSource(const Cfg *Func) const {
   Ostream &Str = Func->getContext()->getStrEmit();
   Variable *Dest = getDest();
   Operand *Src = getSrc(0);
-  auto *S = llvm::dyn_cast<Variable>(Src);
-  Str << "\t";
-  if (Dest->hasReg()) {
-    if (S && S->hasReg())
-      Str << "move";
-    else
-      Str << "lw";
-  } else {
-    if (S && S->hasReg()) {
-      Str << "sw"
-             "\t";
-      getSrc(0)->emit(Func);
-      Str << ", ";
+  auto *SrcV = llvm::dyn_cast<Variable>(Src);
+
+  const char *ActualOpcode = NULL;

[Jim Stichnoth, 2016/07/06 11:18:57]

nullptr

[obucinac, 2016/07/06 13:40:53]

Done.

+  bool DestIsReg = Dest->hasReg();

[Jim Stichnoth, 2016/07/06 11:18:57]

const bool
for these 4 variables

[obucinac, 2016/07/06 13:40:53]

Done.

+  bool DestIsMem = !Dest->hasReg();
+  bool SrcIsReg = (SrcV && SrcV->hasReg());
+  bool SrcIsMem = !(SrcV && SrcV->hasReg());

[Jim Stichnoth, 2016/07/06 11:18:57]

Just checking - what is Src is a Constant, e.g. "42"?
Is that guaranteed to be legalized away during lowering?
Should that be asserted here?

[obucinac, 2016/07/06 13:40:53]

All constants are lowered to OperandMIPS32Mem. We may add assert check anyway.
Mohit covers this.

+
+  // reg to reg
+  if (DestIsReg && SrcIsReg) {
+    switch (Dest->getType()) {
+    default:
+      UnimplementedError(getFlags());
+    case IceType_f32:
+      ActualOpcode = "mov.s";
+      break;
+    case IceType_f64:
+      ActualOpcode = "mov.d";
+      break;
+    case IceType_i32:
+      Str << "\t";

[Jim Stichnoth, 2016/07/06 11:18:58]

Str << "\t" "add" "\t";
to use compile-time string concatenation when possible.

In case it's not clear, I prefer not to use "\tadd\t" because then you can find
these strings more easily using
  git grep -w add
  git grep '"add"'

[obucinac, 2016/07/06 13:40:53]

Done.

+      Str << "add";
+      Str << "\t";
       getDest()->emit(Func);
+      Str << ", $zero, ";
+      getSrc(0)->emit(Func);
       return;
-    } else
-      Str << "move";
+    case IceType_i64: {
+      UnimplementedError(getFlags());
+      break;
+    }
+    case IceType_v4i1:
+    case IceType_v8i1:
+    case IceType_v16i1:
+    case IceType_v16i8:
+    case IceType_v8i16:
+    case IceType_v4i32:
+    case IceType_v4f32: {
+      UnimplementedError(getFlags());
+      break;
+    }
+    }
+
+    Str << "\t";

[Jim Stichnoth, 2016/07/06 11:18:57]

Str << "\t" << ActualOpcode << "\t";
here and below

[obucinac, 2016/07/06 13:40:53]

Done.

+    Str << ActualOpcode;
+    Str << "\t";
+    getDest()->emit(Func);
+    Str << ", ";
+    getSrc(0)->emit(Func);
+    return;
   }
 
-  Str << "\t";
-  getDest()->emit(Func);
-  Str << ", ";
-  getSrc(0)->emit(Func);
+  // reg to stack
+  if (DestIsMem && SrcIsReg) {
+    switch (Dest->getType()) {
+    default:
+      UnimplementedError(getFlags());

[Jim Stichnoth, 2016/07/06 11:18:58]

add "break" after this?

[obucinac, 2016/07/06 13:40:53]

Done.

+    case IceType_f32:
+      ActualOpcode = "swc1";
+      break;
+    case IceType_f64:
+      ActualOpcode = "sdc1";
+      break;
+    case IceType_i32:
+      ActualOpcode = "sw";
+      break;
+    case IceType_i64: {
+      UnimplementedError(getFlags());
+      break;
+    }
+    case IceType_v4i1:
+    case IceType_v8i1:
+    case IceType_v16i1:
+    case IceType_v16i8:
+    case IceType_v8i16:
+    case IceType_v4i32:
+    case IceType_v4f32: {
+      UnimplementedError(getFlags());
+      break;
+    }
+    }
+
+    Str << "\t";
+    Str << ActualOpcode;
+    Str << "\t";
+    getSrc(0)->emit(Func);
+    Str << ", ";
+    getDest()->emit(Func);
+    return;
+  }
+
+  // stack to reg
+  if (DestIsReg && SrcIsMem) {
+    switch (Dest->getType()) {
+    default:
+      UnimplementedError(getFlags());
+    case IceType_f32:
+      ActualOpcode = "lwc1";
+      break;
+    case IceType_f64:
+      ActualOpcode = "ldc1";
+      break;
+    case IceType_i32:
+      ActualOpcode = "lw";
+      break;
+    case IceType_i64: {
+      UnimplementedError(getFlags());
+      break;
+    }
+    case IceType_v4i1:
+    case IceType_v8i1:
+    case IceType_v16i1:
+    case IceType_v16i8:
+    case IceType_v8i16:
+    case IceType_v4i32:
+    case IceType_v4f32: {
+      UnimplementedError(getFlags());
+      break;
+    }
+    }
+
+    Str << "\t";
+    Str << ActualOpcode;
+    Str << "\t";
+    getDest()->emit(Func);
+    Str << ", ";
+    getSrc(0)->emit(Func);
+    return;
+  }
+
+  // stack to stack
+  UnimplementedError(getFlags());

[Jim Stichnoth, 2016/07/06 11:18:57]

Should this be a hard error?  E.g. in x86 it would be, because mem-to-mem moves
are generally not possible and so the lowering should have legalized it to
something valid.

[obucinac, 2016/07/06 13:40:53]

This case will be covered in lowerAssign, by copying through temporary register.
Currently, no test case emits mov with two stack operands (although in previous
version there is a case for it).

 }
 
 } // end of namespace MIPS32