[Mips64] Fix extension of 32-bit integer types.

[vradosavljevic]

Summary

For Mips64 N32/N64 ABI, unsigned 32-bit integers should be sign extended. This patch also fixes cases where operand for truncate instruction is in another basic block.

[vradosavljevic]

TruncNoExt node is used for values smaller than 32bit, where we don't need to do sign extension.

[dsanders]

Could you point me at the original problem you're trying to solve? The new node appears to be doing the same job as AssertZExt and I'm fairly certain this is headed into a bigger problem we encountered back in spring.

The summary of the larger bug is that MIPS64 lacks a 32-bit comparison instruction (the same opcode performs a 64-bit comparison) but the code generator isn't properly aware of this. As a result, the code generator expects the hardware to ignore bits 32-63 but they actually affect the result. This bug gets exposed as we increase our reliance on the code generator getting it right by removing the redundant extends.

@vkalintiris has been working on a fix to the larger problem (D10970 and a second part to handle the operands). Once both are committed we should be able to safely remove any redundant sign extends that remain.

lib/Target/Mips/MipsISelLowering.cpp

2909–2910

Our calling convention doesn't really matter at this point since clang is responsible for emitting appropriate signext/zeroext for the given ABI. We just need to follow what the IR says.

2927–2933

AssertZext already eliminates any extends the the TRUNCATE would otherwise introduce. I suspect something else in the DAG is causing the sign extends you're trying to remove.

test/CodeGen/Mips/divrem.ll

76–186

Please leave these as zeroext. They use zeroext to avoid noise around the interesting generated code and there's no need to match the standard ABI in this kind of test.

test/CodeGen/Mips/mips64-sign-extend.ll

6

This will match any i32->i64 sign extend in the output file, even in different functions. You need some CHECK-LABEL's to restrict the scope of the matching.

31

As above, this could match in any function.

38–45

These CHECK-NOT's don't do what you expect if the earlier CHECK's match in an unexpected function.

test/CodeGen/Mips/octeon_popcnt.ll

24

Please leave these as zeroext. They use zeroext to avoid noise around the interesting generated code and there's no need to match the standard ABI in this kind of test.

[vradosavljevic]

In D15047#297972, @dsanders wrote:

Could you point me at the original problem you're trying to solve? The new node appears to be doing the same job as AssertZExt and I'm fairly certain this is headed into a bigger problem we encountered back in spring.

Original problem is where operand for truncate instruction is in another basic block (foo1 function in mips64-sign-extend.ll). SelectionDAG for this case (AssertZext node was created in SelectionDAGBuilder.cpp (RegsForValue::getCopyFromRegs)):

Optimized legalized selection DAG: BB#1 'foo:if.then'
SelectionDAG has 13 nodes:
  t0: ch = EntryToken
            t2: i64,ch = CopyFromReg t0, Register:i64 %vreg0
          t4: i64 = AssertZext t2, ValueType:ch:i32
        t5: i32 = truncate t4
      t20: i32 = setcc t5, Constant:i32<-1>, setgt:ch
    t12: ch = brcond t0, t20, BasicBlock:ch<if.else 0x14ad3e0>
  t14: ch = br t12, BasicBlock:ch<if.then4 0x14ad318>

In this case, we need to do sign extension for (trunc (assertzext GPR64:$src)), so I changed this pattern to do sign extension. After this change, for arguments zeroext i1, zeroext i8 and zeroext i16, we got redundant SLL instruction. TruncNoExt node is used to prevent generating SLL instruction in these cases.

[dsanders]

This is the same problem Vasileios ran into when he removed some redundant sign extends. The root of the problem is that the legalized SelectionDAG isn't actually legal for MIPS64 since we don't have any 32-bit comparison operations (setlt and friends compare GPR-width values and produce a GPR-width 0 or 1). The legalizer should be promoting both the operands and result of the setcc to i64 which will result in the following DAG:

t0: ch = EntryToken
            t2: i64,ch = CopyFromReg t0, Register:i64 %vreg0
          t4: i64 = AssertZext t2, ValueType:ch:i32
        t5: i32 = truncate t4
      t6: i64 = sext t5
    t20: i64 = setcc t6, Constant:i64<-1>, setgt:ch
  t12: ch = brcond t0, t20, BasicBlock:ch<if.else 0x14ad3e0>
t14: ch = br t12, BasicBlock:ch<if.then4 0x14ad318>

the t6: i64 = sext t5 will then lead to a sll instruction.

One other thing to mention is that the redundant SLL instructions you mention in:

After this change, for arguments zeroext i1, zeroext i8 and zeroext i16, we got redundant SLL instruction.

are needed to promote the i8 to an i32 (which as noted above, should be an i64) for use in the signed comparison.

[vradosavljevic]

In D15047#298514, @dsanders wrote:

This is the same problem Vasileios ran into when he removed some redundant sign extends. The root of the problem is that the legalized SelectionDAG isn't actually legal for MIPS64 since we don't have any 32-bit comparison operations (setlt and friends compare GPR-width values and produce a GPR-width 0 or 1). The legalizer should be promoting both the operands and result of the setcc to i64 which will result in the following DAG:

I applied D10970, but i got compiler error:
llvm/lib/Target/Mips/Mips64r6InstrInfo.td:134:11: error: Couldn't find multiclass 'Cmp_Pats'

What should we do about the problem where operand for truncate instruction is in another basic block?

[vkalintiris]

In D15047#298544, @vradosavljevic wrote:

In D15047#298514, @dsanders wrote:

This is the same problem Vasileios ran into when he removed some redundant sign extends. The root of the problem is that the legalized SelectionDAG isn't actually legal for MIPS64 since we don't have any 32-bit comparison operations (setlt and friends compare GPR-width values and produce a GPR-width 0 or 1). The legalizer should be promoting both the operands and result of the setcc to i64 which will result in the following DAG:

I applied D10970, but i got compiler error:
llvm/lib/Target/Mips/Mips64r6InstrInfo.td:134:11: error: Couldn't find multiclass 'Cmp_Pats'

You'll have to apply D14612 first. I updated the list of dependencies in D10970 to reflect that.

[dsanders]

In D15047#298544, @vradosavljevic wrote:

In D15047#298514, @dsanders wrote:

This is the same problem Vasileios ran into when he removed some redundant sign extends. The root of the problem is that the legalized SelectionDAG isn't actually legal for MIPS64 since we don't have any 32-bit comparison operations (setlt and friends compare GPR-width values and produce a GPR-width 0 or 1). The legalizer should be promoting both the operands and result of the setcc to i64 which will result in the following DAG:

I applied D10970, but i got compiler error:
llvm/lib/Target/Mips/Mips64r6InstrInfo.td:134:11: error: Couldn't find multiclass 'Cmp_Pats'

It depends on D14612 so you need to apply that first.

What should we do about the problem where operand for truncate instruction is in another basic block?

It's the same issue, the sign extend is missing because our legalized SelectionDAG is actually illegal for MIPS64. It will be fixed by promoting the setcc to i64.

[dsanders]

It depends on D14612 so you need to apply that first.

Nearly forgot to say: This patch is only half the solution. We also need to promote the operands of setcc nodes.

[vradosavljevic]

Abandoned in favour of http://reviews.llvm.org/D18893 .

lib/Target/Mips/Mips64InstrInfo.td

 // setcc patterns
 defm : SeteqPats<GPR64, SLTiu64, XOR64, SLTu64, ZERO_64>;
 defm : SetlePats<GPR64, SLT64, SLTu64>;
 defm : SetgtPats<GPR64, SLT64, SLTu64>;
 defm : SetgePats<GPR64, SLT64, SLTu64>;
 defm : SetgeImmPats<GPR64, SLTi64, SLTiu64>;
 
 // truncate
+// Truncation from i64 to i32 with no zero/sign extension.
+def Mips64TruncNoExt : SDNode<"MipsISD::TruncNoExt", SDTIntTruncOp>;
+
 def : MipsPat<(trunc (assertsext GPR64:$src)),
               (EXTRACT_SUBREG GPR64:$src, sub_32)>;
-def : MipsPat<(trunc (assertzext GPR64:$src)),
+def : MipsPat<(Mips64TruncNoExt GPR64:$src),
               (EXTRACT_SUBREG GPR64:$src, sub_32)>;
+def : MipsPat<(trunc (assertzext GPR64:$src)),
+              (SLL (EXTRACT_SUBREG GPR64:$src, sub_32), 0)>;
 def : MipsPat<(i32 (trunc GPR64:$src)),
               (SLL (EXTRACT_SUBREG GPR64:$src, sub_32), 0)>;
 
 // variable shift instructions patterns
 def : MipsPat<(shl GPR64:$rt, (i32 (trunc GPR64:$rs))),
               (DSLLV GPR64:$rt, (EXTRACT_SUBREG GPR64:$rs, sub_32))>;
 def : MipsPat<(srl GPR64:$rt, (i32 (trunc GPR64:$rs))),
               (DSRLV GPR64:$rt, (EXTRACT_SUBREG GPR64:$rs, sub_32))>;

lib/Target/Mips/MipsISelLowering.h

enum NodeType : unsigned {
 
       // Floating Point Conditional Moves
       CMovFP_T,
       CMovFP_F,
 
       // FP-to-int truncation node.
       TruncIntFP,
 
+      // Node used to truncate from i64 to i32 with no zero/sign extension.
+      // This is only used for N32/N64 ABI.
+      TruncNoExt,
+
       // Return
       Ret,
 
       // Interrupt, exception, error trap Return
       ERet,
 
       // Software Exception Return.
       EH_RETURN,

lib/Target/Mips/MipsISelLowering.cpp

SDValue MipsTargetLowering::getTargetNode(ConstantPoolSDNode *N, EVT Ty,
                                           unsigned Flag) const {
   return DAG.getTargetConstantPool(N->getConstVal(), Ty, N->getAlignment(),
                                    N->getOffset(), Flag);
 }
 
 const char *MipsTargetLowering::getTargetNodeName(unsigned Opcode) const {
   switch ((MipsISD::NodeType)Opcode) {
   case MipsISD::FIRST_NUMBER:      break;
+  case MipsISD::TruncNoExt:        return "MipsISD::TruncNoExt";
   case MipsISD::JmpLink:           return "MipsISD::JmpLink";
   case MipsISD::TailCall:          return "MipsISD::TailCall";
   case MipsISD::Hi:                return "MipsISD::Hi";
   case MipsISD::Lo:                return "MipsISD::Lo";
   case MipsISD::GPRel:             return "MipsISD::GPRel";
   case MipsISD::ThreadPointer:     return "MipsISD::ThreadPointer";
   case MipsISD::Ret:               return "MipsISD::Ret";
   case MipsISD::ERet:              return "MipsISD::ERet";
for (unsigned i = 0; i != RVLocs.size(); ++i) {
 
     InVals.push_back(Val);
   }
 
   return Chain;
 }
 
 static SDValue UnpackFromArgumentSlot(SDValue Val, const CCValAssign &VA,
-                                      EVT ArgVT, SDLoc DL, SelectionDAG &DAG) {
+                                      EVT ArgVT, SDLoc DL, SelectionDAG &DAG,
+                                      const MipsABIInfo &ABI) {
   MVT LocVT = VA.getLocVT();
   EVT ValVT = VA.getValVT();
 
   // Shift into the upper bits if necessary.
   switch (VA.getLocInfo()) {
   default:
     break;
   case CCValAssign::AExtUpper:
   case CCValAssign::SExtUpper:
   case CCValAssign::ZExtUpper: {
     unsigned ValSizeInBits = ArgVT.getSizeInBits();
     unsigned LocSizeInBits = VA.getLocVT().getSizeInBits();
     unsigned Opcode =
         VA.getLocInfo() == CCValAssign::ZExtUpper ? ISD::SRL : ISD::SRA;
     Val = DAG.getNode(
         Opcode, DL, VA.getLocVT(), Val,
         DAG.getConstant(LocSizeInBits - ValSizeInBits, DL, VA.getLocVT()));
     break;
   }
   }
 
   // If this is an value smaller than the argument slot size (32-bit for O32,
   // 64-bit for N32/N64), it has been promoted in some way to the argument slot
   // size. Extract the value and insert any appropriate assertions regarding
-  // sign/zero extension.
+  // sign/zero extension. In N32/N64 ABI unsigned 32-bit integers are
+  // represented in a 64-bit register as sign-extended value.
   switch (VA.getLocInfo()) {
   default:
     llvm_unreachable("Unknown loc info!");
   case CCValAssign::Full:
     break;
   case CCValAssign::AExtUpper:
   case CCValAssign::AExt:
     Val = DAG.getNode(ISD::TRUNCATE, DL, ValVT, Val);
     break;
   case CCValAssign::SExtUpper:
   case CCValAssign::SExt:
     Val = DAG.getNode(ISD::AssertSext, DL, LocVT, Val, DAG.getValueType(ValVT));
     Val = DAG.getNode(ISD::TRUNCATE, DL, ValVT, Val);
     break;
   case CCValAssign::ZExtUpper:
   case CCValAssign::ZExt:
+    // We don't need to sign extend values smaller than 32bit.
+    if (ABI.AreGprs64bit() && ArgVT.getSimpleVT() < MVT::i32)
+      Val = DAG.getNode(MipsISD::TruncNoExt, DL, ValVT, Val);
+    else {
       Val = DAG.getNode(ISD::AssertZext, DL, LocVT, Val, DAG.getValueType(ValVT));
       Val = DAG.getNode(ISD::TRUNCATE, DL, ValVT, Val);
+    }
     break;
   case CCValAssign::BCvt:
     Val = DAG.getNode(ISD::BITCAST, DL, ValVT, Val);
     break;
   }
 
   return Val;
 }
if (IsRegLoc) {
       unsigned ArgReg = VA.getLocReg();
       const TargetRegisterClass *RC = getRegClassFor(RegVT);
 
       // Transform the arguments stored on
       // physical registers into virtual ones
       unsigned Reg = addLiveIn(DAG.getMachineFunction(), ArgReg, RC);
       SDValue ArgValue = DAG.getCopyFromReg(Chain, DL, Reg, RegVT);
 
-      ArgValue = UnpackFromArgumentSlot(ArgValue, VA, Ins[i].ArgVT, DL, DAG);
+      ArgValue = UnpackFromArgumentSlot(ArgValue, VA, Ins[i].ArgVT, DL, DAG, ABI);
 
       // Handle floating point arguments passed in integer registers and
       // long double arguments passed in floating point registers.
       if ((RegVT == MVT::i32 && ValVT == MVT::f32) ||
           (RegVT == MVT::i64 && ValVT == MVT::f64) ||
           (RegVT == MVT::f64 && ValVT == MVT::i64))
         ArgValue = DAG.getNode(ISD::BITCAST, DL, ValVT, ArgValue);
       else if (ABI.IsO32() && RegVT == MVT::i32 &&
if (IsRegLoc) {
       // Create load nodes to retrieve arguments from the stack
       SDValue FIN = DAG.getFrameIndex(FI, getPointerTy(DAG.getDataLayout()));
       SDValue ArgValue = DAG.getLoad(
           LocVT, DL, Chain, FIN,
           MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI),
           false, false, false, 0);
       OutChains.push_back(ArgValue.getValue(1));
 
-      ArgValue = UnpackFromArgumentSlot(ArgValue, VA, Ins[i].ArgVT, DL, DAG);
+      ArgValue = UnpackFromArgumentSlot(ArgValue, VA, Ins[i].ArgVT, DL, DAG, ABI);
 
       InVals.push_back(ArgValue);
     }
   }
 
   for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
     // The mips ABIs for returning structs by value requires that we copy
     // the sret argument into $v0 for the return. Save the argument into

test/CodeGen/Mips/divrem.ll

 ; GPR32-TRAP:    teq $5, $zero, 7
 
 ; GPR64:         mod $2, $4, $5
 ; GPR64-TRAP:    teq $5, $zero, 7
 
 ; NOCHECK-NOT:   teq
 
 ; ACC32:         mfhi $2
 ; ACC64:         mfhi $2
 
 ; ALL: .end srem1
 
   %rem = srem i32 %a0, %a1
   ret i32 %rem
 }
 
-define i32 @udiv1(i32 zeroext %a0, i32 zeroext %a1) nounwind readnone {
+define i32 @udiv1(i32 signext %a0, i32 signext %a1) nounwind readnone {
 entry:
 ; ALL-LABEL: udiv1:
 
 ; ACC32:         divu $zero, $4, $5
 ; ACC32-TRAP:    teq $5, $zero, 7
 
 ; ACC64:         divu $zero, $4, $5
 ; ACC64-TRAP:    teq $5, $zero, 7
 
 ; GPR32:         divu $2, $4, $5
 ; GPR32-TRAP:    teq $5, $zero, 7
 
 ; GPR64:         divu $2, $4, $5
 ; GPR64-TRAP:    teq $5, $zero, 7
 
 ; NOCHECK-NOT:   teq
 
 ; ACC32:         mflo $2
 ; ACC64:         mflo $2
 
 ; ALL: .end udiv1
   %div = udiv i32 %a0, %a1
   ret i32 %div
 }
 
-define i32 @urem1(i32 zeroext %a0, i32 zeroext %a1) nounwind readnone {
+define i32 @urem1(i32 signext %a0, i32 signext %a1) nounwind readnone {
 entry:
 ; ALL-LABEL: urem1:
 
 ; ACC32:         divu $zero, $4, $5
 ; ACC32-TRAP:    teq $5, $zero, 7
 
 ; ACC64:         divu $zero, $4, $5
 ; ACC64-TRAP:    teq $5, $zero, 7
 
 ; GPR32:         modu $2, $4, $5
 ; GPR32-TRAP:    teq $5, $zero, 7
 
 ; GPR64:         modu $2, $4, $5
 ; GPR64-TRAP:    teq $5, $zero, 7
 
 ; NOCHECK-NOT:   teq
 
 ; ACC32:         mfhi $2
 ; ACC64:         mfhi $2
 
 ; ALL: .end urem1
 
   %rem = urem i32 %a0, %a1
   ret i32 %rem
 }
 
 define i32 @sdivrem1(i32 signext %a0, i32 signext %a1, i32* nocapture %r) nounwind {
 entry:
 ; ALL-LABEL: sdivrem1:
 
 ; ACC32:         div $zero, $4, $5
 ; ACC32-TRAP:    teq $5, $zero, 7
 ; NOCHECK-NOT:   teq
 ; ACC32:         mflo $2
 ; ACC32:         mfhi $[[R0:[0-9]+]]
 ; ACC32:         sw $[[R0]], 0(${{[0-9]+}})
 
 ; ACC64:         div $zero, $4, $5
 ; ACC64-TRAP:    teq $5, $zero, 7
 ; NOCHECK-NOT:   teq
 ; ACC64:         mflo $2
 ; ACC64:         mfhi $[[R0:[0-9]+]]
 ; ACC64:         sw $[[R0]], 0(${{[0-9]+}})
 
 ; GPR32:         mod $[[R0:[0-9]+]], $4, $5
 ; GPR32-TRAP:    teq $5, $zero, 7
 ; NOCHECK-NOT:   teq
 ; GPR32:         sw $[[R0]], 0(${{[0-9]+}})
 ; GPR32-DAG:     div $2, $4, $5
 ; GPR32-TRAP:    teq $5, $zero, 7
 
 ; GPR64:         mod $[[R0:[0-9]+]], $4, $5
 ; GPR64-TRAP:    teq $5, $zero, 7
 ; NOCHECK-NOT:   teq
 ; GPR64:         sw $[[R0]], 0(${{[0-9]+}})
 ; GPR64-DAG:     div $2, $4, $5
 ; GPR64-TRAP:    teq $5, $zero, 7
 ; NOCHECK-NOT:   teq
 
 ; ALL: .end sdivrem1
 
   %rem = srem i32 %a0, %a1
   store i32 %rem, i32* %r, align 4
   %div = sdiv i32 %a0, %a1
   ret i32 %div
 }
 
-define i32 @udivrem1(i32 zeroext %a0, i32 zeroext %a1, i32* nocapture %r) nounwind {
+define i32 @udivrem1(i32 signext %a0, i32 signext %a1, i32* nocapture %r) nounwind {
 entry:
 ; ALL-LABEL: udivrem1:
 
 ; ACC32:         divu $zero, $4, $5
 ; ACC32-TRAP:    teq $5, $zero, 7
 ; NOCHECK-NOT:   teq
 ; ACC32:         mflo $2
 ; ACC32:         mfhi $[[R0:[0-9]+]]
 ; ACC32:         sw $[[R0]], 0(${{[0-9]+}})
 
 ; ACC64:         divu $zero, $4, $5
 ; ACC64-TRAP:    teq $5, $zero, 7
 ; NOCHECK-NOT:   teq
 ; ACC64:         mflo $2
 ; ACC64:         mfhi $[[R0:[0-9]+]]
 ; ACC64:         sw $[[R0]], 0(${{[0-9]+}})

test/CodeGen/Mips/mips64-sign-extend.ll

+; RUN: llc  < %s -march=mips64el -mcpu=mips64r2 -target-abi=n64 | FileCheck %s
+; RUN: llc  < %s -march=mips64el -mcpu=mips64r2 -target-abi=n32 | FileCheck %s
+
+define i64 @foo1(i64 zeroext %var) {
+entry:
+; CHECK: sll ${{[0-9]+}}, ${{[0-9]+}}, 0
+  %shr = lshr i64 %var, 32
+  %cmp = icmp eq i64 %shr, 0
+  br i1 %cmp, label %if.end6, label %if.then
+
+if.then:                                          ; preds = %entry
+  %conv = trunc i64 %shr to i32
+  %cmp2 = icmp slt i32 %conv, 0
+  br i1 %cmp2, label %if.then4, label %if.else
+
+if.then4:                                         ; preds = %if.then
+  %add = add i64 %var, 16
+  br label %if.end6
+
+if.else:                                          ; preds = %if.then
+  %add5 = add i64 %var, 32
+  br label %if.end6
+
+if.end6:                                          ; preds = %entry, %if.then4, %if.else
+  %var.addr.0 = phi i64 [ %add, %if.then4 ], [ %add5, %if.else ], [ %var, %entry ]
+  ret i64 %var.addr.0
+}
+
+define i32 @foo2(i32 zeroext %var) #0 {
+entry:
+; CHECK: sll ${{[0-9]+}}, ${{[0-9]+}}, 0
+  %add = add i32 %var, 5
+  ret i32 %add
+}
+
+define i16 @foo3(i16 zeroext %var) #0 {
+entry:
+; CHECK-NOT: sll ${{[0-9]+}}, ${{[0-9]+}}, 0
+  %add = add i16 %var, 5
+  ret i16 %add
+}
+
+define i8 @foo4(i8 zeroext %var) #0 {
+entry:
+; CHECK-NOT: sll ${{[0-9]+}}, ${{[0-9]+}}, 0
+  %add = add i8 %var, 5
+  ret i8 %add
+}

test/CodeGen/Mips/octeon_popcnt.ll

define i16 @cnt16(i16 %x) nounwind readnone {
   ret i16 %cnt
 ; OCTEON-LABEL: cnt16:
 ; OCTEON: jr   $ra
 ; OCTEON: pop  $2, [[R1:\$[0-9]+]]
 ; MIPS64-LABEL: cnt16:
 ; MIPS64-NOT: pop
 }
 
-define i32 @cnt32(i32 zeroext %x) nounwind readnone {
+define i32 @cnt32(i32 signext %x) nounwind readnone {
   %cnt = tail call i32 @llvm.ctpop.i32(i32 %x)
   ret i32 %cnt
 ; OCTEON-LABEL: cnt32:
 ; OCTEON: jr   $ra
 ; OCTEON: pop  $2, $4
 ; MIPS64-LABEL: cnt32:
 ; MIPS64-NOT: pop
 }