[SubZero] Fix f64 to/from i64 moves

src/IceTargetLoweringMIPS32.cpp

Index: src/IceTargetLoweringMIPS32.cpp
diff --git a/src/IceTargetLoweringMIPS32.cpp b/src/IceTargetLoweringMIPS32.cpp
index 69080e7d2ddfbf3f233efceadc54c43d084f92a6..219b2506d0fe254c14599d0a79b348936af433a0 100644
--- a/src/IceTargetLoweringMIPS32.cpp
+++ b/src/IceTargetLoweringMIPS32.cpp
@@ -116,7 +116,16 @@ void TargetMIPS32::assignVarStackSlots(VarList &SortedSpilledVariables,
   CfgVector<size_t> LocalsSize(Func->getNumNodes());
   const bool SimpleCoalescing = !callsReturnsTwice();
 
-  for (Variable *Var : SortedSpilledVariables) {
+  // We have sorted the spilled variables based on their alignment.

[Jim Stichnoth, 2016/10/19 18:41:16]

Reflow comment to 80-col.  (M-q if using emacs...)

+  // However when the alignment is same then it is the index of the
+  // variable which gets precedence. In case of i64 variables (which
+  // we split into Hi and Lo parts), the Lo part is created first and
+  // thus it gets a lower index than the Hi part. Due to this Lo part
+  // is allocated on stack before its corresponding Hi part.
+  // As per the MIPS32 ABI, Hi part must be stored in the high address.
+  // We assign stack slots to the variables in reverse order
+  // so that Hi part can get high address.
+  for (Variable *Var : reverse_range(SortedSpilledVariables)) {

[Jim Stichnoth, 2016/10/19 18:41:16]

This fix concerns me a bit.

The first concern is how this interacts with the register allocator.  On x8632
(and mips32 as well I assume), the hi and lo parts are independently
register-allocated, so either or both parts may end up in registers or on the
stack.  Also in x8632, the lowering is arranged such that bitcasts between i64
and double doesn't rely on the i32 components being adjacent on the stack.  So
it's not clear to me where the hi/lo stack ordering comes into play here.

Maybe the issue is just with respect to function arguments passed on the stack?

The other concern is about the sorting.  SortedSpilledVariables is deliberately
sorted first by alignment (largest to smallest), and in the case of ties,
deterministically sorted by variable number (smallest to largest).  It's
important that it is sorted by largest-to-smallest alignment, since assuming all
alignment values are powers of two, it guarantees that aligning the first one
(with the largest alignment requirement) will also make everything else aligned.
 If you reverse this order, the guarantee no longer holds.

[jaydeep.patil, 2016/10/20 04:35:40]

The issue occurs when a spilled i64 is moved to f64. On MIPS we split i64 into
Hi-Lo, but f64 is a single 64-bit variable. To move a spilled i64 to f64 we need
a 64-bit load of f64 from Lo part of i64. However the Lo part is stored at
high-address and its Hi part at low-address which is exactly opposite of the
ABI. 

As mentioned by you, reverse allocation is not a good idea. I should not rely on
the order of the variable on stack.

Let me change it to use registers instead and handle it in post-lower
legalization.

     size_t Increment = typeWidthInBytesOnStack(Var->getType());
     if (SimpleCoalescing && VMetadata->isTracked(Var)) {
       if (VMetadata->isMultiBlock(Var)) {
@@ -1775,12 +1784,7 @@ void TargetMIPS32::PostLoweringLegalizer::legalizeMov(InstMIPS32Mov *MovInstr) {
     auto *SrcR = llvm::cast<Variable>(Src);
     assert(SrcR->hasReg());
     assert(!SrcR->isRematerializable());
-    int32_t Offset = 0;
-
-    if (MovInstr->getDestHi() != nullptr)
-      Offset = MovInstr->getDestHi()->getStackOffset();
-    else
-      Offset = Dest->getStackOffset();
+    int32_t Offset = Dest->getStackOffset();
 
     // This is a _mov(Mem(), Variable), i.e., a store.
     auto *Base = Target->getPhysicalRegister(Target->getFrameOrStackReg());
@@ -1813,8 +1817,8 @@ void TargetMIPS32::PostLoweringLegalizer::legalizeMov(InstMIPS32Mov *MovInstr) {
           (llvm::cast<Variable>(MovInstr->getSrc(0)))->getRegNum();
       const auto SecondReg =
           (llvm::cast<Variable>(MovInstr->getSrc(1)))->getRegNum();
-      Variable *SrcGPRHi = Target->makeReg(IceType_i32, SecondReg);
-      Variable *SrcGPRLo = Target->makeReg(IceType_i32, FirstReg);
+      Variable *SrcGPRHi = Target->makeReg(IceType_i32, FirstReg);
+      Variable *SrcGPRLo = Target->makeReg(IceType_i32, SecondReg);
       OperandMIPS32Mem *AddrHi = OperandMIPS32Mem::create(
           Target->Func, DestTy, Base,
           llvm::cast<ConstantInteger32>(
@@ -2536,7 +2540,15 @@ void TargetMIPS32::lowerArithmetic(const InstArithmetic *Instr) {
     return;
   }
   case InstArithmetic::Lshr: {
-    _srlv(T, Src0R, Src1R);
+    auto *T0R = Src0R;
+    auto *T1R = Src1R;
+    if (Dest->getType() != IceType_i32) {
+      T0R = makeReg(IceType_i32);
+      lowerCast(InstCast::create(Func, InstCast::Zext, T0R, Src0R));
+      T1R = makeReg(IceType_i32);
+      lowerCast(InstCast::create(Func, InstCast::Zext, T1R, Src1R));
+    }
+    _srlv(T, T0R, T1R);
     _mov(Dest, T);
     return;
   }
@@ -2547,8 +2559,16 @@ void TargetMIPS32::lowerArithmetic(const InstArithmetic *Instr) {
   }
   case InstArithmetic::Udiv: {
     auto *T_Zero = I32Reg(RegMIPS32::Reg_ZERO);
-    _divu(T_Zero, Src0R, Src1R);
-    _teq(Src1R, T_Zero, DivideByZeroTrapCode); // Trap if divide-by-zero
+    auto *T0R = Src0R;
+    auto *T1R = Src1R;
+    if (Dest->getType() != IceType_i32) {
+      T0R = makeReg(IceType_i32);
+      lowerCast(InstCast::create(Func, InstCast::Zext, T0R, Src0R));
+      T1R = makeReg(IceType_i32);
+      lowerCast(InstCast::create(Func, InstCast::Zext, T1R, Src1R));
+    }
+    _divu(T_Zero, T0R, T1R);
+    _teq(T1R, T_Zero, DivideByZeroTrapCode); // Trap if divide-by-zero
     _mflo(T, T_Zero);
     _mov(Dest, T);
     return;
@@ -2563,8 +2583,16 @@ void TargetMIPS32::lowerArithmetic(const InstArithmetic *Instr) {
   }
   case InstArithmetic::Urem: {
     auto *T_Zero = I32Reg(RegMIPS32::Reg_ZERO);
-    _divu(T_Zero, Src0R, Src1R);
-    _teq(Src1R, T_Zero, DivideByZeroTrapCode); // Trap if divide-by-zero
+    auto *T0R = Src0R;
+    auto *T1R = Src1R;
+    if (Dest->getType() != IceType_i32) {
+      T0R = makeReg(IceType_i32);
+      lowerCast(InstCast::create(Func, InstCast::Zext, T0R, Src0R));
+      T1R = makeReg(IceType_i32);
+      lowerCast(InstCast::create(Func, InstCast::Zext, T1R, Src1R));
+    }
+    _divu(T_Zero, T0R, T1R);
+    _teq(T1R, T_Zero, DivideByZeroTrapCode); // Trap if divide-by-zero
     _mfhi(T, T_Zero);
     _mov(Dest, T);
     return;
@@ -3348,20 +3376,24 @@ void TargetMIPS32::lowerCast(const InstCast *Instr) {
       return;
     }
     if (Src0Ty != IceType_i64) {
+      Variable *Src0R = legalizeToReg(Src0);
+      auto *T0R = Src0R;
+      if (Src0Ty != IceType_i32 && CastKind == InstCast::Uitofp) {
+        T0R = makeReg(IceType_i32);
+        lowerCast(InstCast::create(Func, InstCast::Zext, T0R, Src0R));
+      }
       if (isScalarIntegerType(Src0Ty) && DestTy == IceType_f32) {
-        Variable *Src0R = legalizeToReg(Src0);
         Variable *FTmp1 = makeReg(IceType_f32);
         Variable *FTmp2 = makeReg(IceType_f32);
-        _mtc1(FTmp1, Src0R);
+        _mtc1(FTmp1, T0R);
         _cvt_s_w(FTmp2, FTmp1);
         _mov(Dest, FTmp2);
         return;
       }
       if (isScalarIntegerType(Src0Ty) && DestTy == IceType_f64) {
-        Variable *Src0R = legalizeToReg(Src0);
         Variable *FTmp1 = makeReg(IceType_f64);
         Variable *FTmp2 = makeReg(IceType_f64);
-        _mtc1(FTmp1, Src0R);
+        _mtc1(FTmp1, T0R);
         _cvt_d_w(FTmp2, FTmp1);
         _mov(Dest, FTmp2);
         return;
@@ -4062,9 +4094,6 @@ void TargetMIPS32::lowerIntrinsicCall(const InstIntrinsicCall *Instr) {
   Variable *Dest = Instr->getDest();
   Type DestTy = (Dest == nullptr) ? IceType_void : Dest->getType();
   switch (Instr->getIntrinsicInfo().ID) {
-  default:
-    llvm::report_fatal_error("Unexpected intrinsic");
-    return;
   case Intrinsics::AtomicCmpxchg: {
     UnimplementedLoweringError(this, Instr);
     return;