[SubZero] Fix f64 to/from i64 moves

src/IceTargetLoweringMIPS32.cpp

 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// \brief Implements the TargetLoweringMIPS32 class, which consists almost
@@ skipping 98 matching lines @@
 void TargetMIPS32::assignVarStackSlots(VarList &SortedSpilledVariables,
                                        size_t SpillAreaPaddingBytes,
                                        size_t SpillAreaSizeBytes,
                                        size_t GlobalsAndSubsequentPaddingSize) {
   const VariablesMetadata *VMetadata = Func->getVMetadata();
   size_t GlobalsSpaceUsed = SpillAreaPaddingBytes;
   size_t NextStackOffset = SpillAreaPaddingBytes;
   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)) {
         GlobalsSpaceUsed += Increment;
         NextStackOffset = GlobalsSpaceUsed;
       } else {
         SizeT NodeIndex = VMetadata->getLocalUseNode(Var)->getIndex();
         LocalsSize[NodeIndex] += Increment;
         NextStackOffset = SpillAreaPaddingBytes +
                           GlobalsAndSubsequentPaddingSize +
@@ skipping 1638 matching lines @@
       }
       MovInstr->setDeleted();
       return;
     }
   }
 
   if (!Dest->hasReg()) {
     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());
 
     OperandMIPS32Mem *Addr = OperandMIPS32Mem::create(
         Target->Func, DestTy, Base,
         llvm::cast<ConstantInteger32>(Target->Ctx->getConstantInt32(Offset)));
 
     // FP arguments are passed in GP reg if first argument is in GP. In this
     // case type of the SrcR is still FP thus we need to explicitly generate sw
@@ skipping 12 matching lines @@
           Target->Func, DestTy, Base,
           llvm::cast<ConstantInteger32>(
               Target->Ctx->getConstantInt32(Offset + 4)));
       Target->_sw(SrcGPRLo, Addr);
       Target->_sw(SrcGPRHi, AddrHi);
     } else if (DestTy == IceType_f64 && IsSrcGPReg) {
       const auto FirstReg =
           (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>(
               Target->Ctx->getConstantInt32(Offset + 4)));
       Target->_sw(SrcGPRLo, Addr);
       Target->_sw(SrcGPRHi, AddrHi);
     } else {
       Target->_sw(SrcR, Addr);
     }
 
@@ skipping 701 matching lines @@
     _mul(T, Src0R, Src1R);
     _mov(Dest, T);
     return;
   }
   case InstArithmetic::Shl: {
     _sllv(T, Src0R, Src1R);
     _mov(Dest, T);
     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;
   }
   case InstArithmetic::Ashr: {
     _srav(T, Src0R, Src1R);
     _mov(Dest, T);
     return;
   }
   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;
   }
   case InstArithmetic::Sdiv: {
     auto *T_Zero = I32Reg(RegMIPS32::Reg_ZERO);
     _div(T_Zero, Src0R, Src1R);
     _teq(Src1R, T_Zero, DivideByZeroTrapCode); // Trap if divide-by-zero
     _mflo(T, T_Zero);
     _mov(Dest, T);
     return;
   }
   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;
   }
   case InstArithmetic::Srem: {
     auto *T_Zero = I32Reg(RegMIPS32::Reg_ZERO);
     _div(T_Zero, Src0R, Src1R);
     _teq(Src1R, T_Zero, DivideByZeroTrapCode); // Trap if divide-by-zero
     _mfhi(T, T_Zero);
     _mov(Dest, T);
@@ skipping 763 matching lines @@
     UnimplementedLoweringError(this, Instr);
     break;
   }
   case InstCast::Sitofp:
   case InstCast::Uitofp: {
     if (llvm::isa<Variable64On32>(Dest)) {
       llvm::report_fatal_error("i64-to-fp should have been prelowered.");
       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;
       }
     }
     UnimplementedLoweringError(this, Instr);
     break;
   }
   case InstCast::Bitcast: {
     Operand *Src0 = Instr->getSrc(0);
@@ skipping 680 matching lines @@
     lowerAssign(Assign);
     return;
   }
   llvm::report_fatal_error("InsertElement requires a constant index");
 }
 
 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;
   }
   case Intrinsics::AtomicFence:
     UnimplementedLoweringError(this, Instr);
     return;
   case Intrinsics::AtomicFenceAll:
     // NOTE: FenceAll should prevent and load/store from being moved across the
     // fence (both atomic and non-atomic). The InstMIPS32Mfence instruction is
@@ skipping 1167 matching lines @@
   Str << "\t.set\t"
       << "nomips16\n";
 }
 
 SmallBitVector TargetMIPS32::TypeToRegisterSet[RCMIPS32_NUM];
 SmallBitVector TargetMIPS32::TypeToRegisterSetUnfiltered[RCMIPS32_NUM];
 SmallBitVector TargetMIPS32::RegisterAliases[RegMIPS32::Reg_NUM];
 
 } // end of namespace MIPS32
 } // end of namespace Ice