[PATCH] D10970: [mips] Promote the result of SETCC nodes to GPR width.

[Daniel Sanders via llvm-commits]

dsanders added a comment.

Thanks for your patience on this, I think we're nearly there. Just a couple missed comments and a bit more on the removal of the ZERO_EXTEND_INREG case.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14155
@@ -14154,8 +14154,3 @@
                             N0, N1, CC);
-        if (N2.getValueType().bitsLT(SCC.getValueType()))
-          Temp = DAG.getZeroExtendInReg(SCC, SDLoc(N2),
-                                        N2.getValueType());
-        else
-          Temp = DAG.getNode(ISD::ZERO_EXTEND, SDLoc(N2),
-                             N2.getValueType(), SCC);
+        Temp = DAG.getZExtOrTrunc(SCC, SDLoc(N2), N2.getValueType());
       } else {
----------------
That's not what I'm getting at. Previously, we had two cases ZERO_EXTEND and ZERO_EXTEND_INREG. This patch adds the possibility of TRUNCATE which makes sense to me. However, it also changes the ZERO_EXTEND_INREG cases to ZERO_EXTEND and it's this removal that I don't understand.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:976
@@ -975,3 +975,3 @@
   SDValue Op = GetPromotedInteger(N->getOperand(0));
-  return DAG.getNode(ISD::ANY_EXTEND, SDLoc(N), N->getValueType(0), Op);
+  return DAG.getAnyExtOrTrunc(Op, SDLoc(N), N->getValueType(0));
 }
----------------
vkalintiris wrote:
> dsanders wrote:
> > Could you tell me which case made this necessary?
> The initial problem came from test/Codegen/atomic.ll:
> 
>   t17: i8,i1,ch = AtomicCmpSwapWithSuccess<Volatile LDST1[%ptr]> t0, t2, t14, t16
>     t18: i32 = any_extend t17:1
>   t20: ch,glue = CopyToReg t17:2, Register:i32 %V0, t18
>   t21: ch = MipsISD::Ret t20, Register:i32 %V0, t20:1
> 
> The boolean result of `AtomicCmpSwapWithSuccess` will be promoted to i64 by the call to `GetPromotedInteger()` in the previous line.
> 
> I decided to fix this by consulting `getSetCCResultType()` in the initial SelectionDAG builder when we construct an `AtomicCmpSwapWithSuccess` node.
Ok, that makes sense to me.

================
Comment at: lib/Target/Mips/MicroMipsInstrInfo.td:674-677
@@ -673,6 +673,6 @@
                  ADDI_FM_MM<0x4>;
-  def SLTi_MM  : MMRel, SetCC_I<"slti", setlt, simm16, immSExt16, GPR32Opnd>,
-                 SLTI_FM_MM<0x24>;
-  def SLTiu_MM : MMRel, SetCC_I<"sltiu", setult, simm16, immSExt16, GPR32Opnd>,
-                 SLTI_FM_MM<0x2c>;
+  def SLTi_MM  : MMRel, SetCC_I<"slti", setlt, simm16, immSExt16, GPR32Opnd,
+                 GPR32Opnd>, SLTI_FM_MM<0x24>;
+  def SLTiu_MM : MMRel, SetCC_I<"sltiu", setult, simm16, immSExt16, GPR32Opnd,
+                 GPR32Opnd>, SLTI_FM_MM<0x2c>;
   def ANDi_MM  : MMRel, ArithLogicI<"andi", uimm16, GPR32Opnd>,
----------------
Indentation (GPR32Opnd should line up with the ")

================
Comment at: test/CodeGen/Mips/atomic.ll:351
@@ -348,1 +350,3 @@
+; MIPS64-ANY:    sltiu   $[[R21:[0-9]+]], $[[R20]], 1
+; MIPS64-ANY:    sll     $2, $[[R21]], 0
 }
----------------
Makes sense to me.

================
Comment at: test/CodeGen/Mips/fcmp.ll:36
@@ -35,1 +35,3 @@
+; 64-C-DAG:      movf $[[T0]], $zero, $fcc0
+; 64-C:          sll $2, $[[T0]], 0
 
----------------
Could you comment on this?

================
Comment at: test/CodeGen/Mips/fcmp.ll:45
@@ +44,3 @@
+; 64-CMP-DAG:    dmfc1 $[[T1:[0-9]+]], $[[T0]]
+; 64-CMP-DAG:    sll $[[T2:[0-9]+]], $[[T1]], 0
+; 64-CMP-DAG:    andi $2, $[[T2]], 1
----------------
Could you comment on this?

================
Comment at: test/CodeGen/Mips/llvm-ir/select.ll:158
@@ -163,3 +157,3 @@
 
-define float @tst_select_i1_float(i1 signext %s, float %x, float %y) {
+define float @tst_select_i1_float(i1 %s, float %x, float %y) {
 entry:
----------------
Ok then. I think answers one of my questions for that patch too.


http://reviews.llvm.org/D10970