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

[Vasileios Kalintiris via llvm-commits]

vkalintiris added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14065
@@ -14064,8 +14064,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 {
----------------
dsanders wrote:
> Looking at this again, I notice that getZExtOrTrunc() can only produce ZERO_EXTEND or TRUNCATE. We lose the getZeroExtendInReg() path. Could you bring back the special case?
With this input:

  (select (i64 (setcc ...)), (i32 N1), (i32 N2))

We'll get SCC:i64 and Temp:i64. The ISD::SHL node below will try to create a shift operation with:

  (i32 (shl i64, i32))

which is not valid. Given that the boolean contents are either all 0 or all 1, we can use getZextOrTrunc(), right?

================
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));
 }
----------------
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.

================
Comment at: lib/Target/Mips/Mips64r6InstrInfo.td:78-79
@@ +77,4 @@
+class SEL64_S_DESC : COP1_SEL_DESC_BASE<"sel.s", FGRCC64Opnd, FGR32Opnd> {
+  // We must copy FGRCC64Opnd's subreg into FGR32.
+  bit usesCustomInserter = 1;
+}
----------------
dsanders wrote:
> > This is because operands are still 32-bit instead of GPR-width, right?
> 
> Could you reply to this comment?
Sorry, I missed this. You're correct, that's why we have to use a custom inserter.

================
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
 }
----------------
dsanders wrote:
> Ah ok. This is the problem you showed me last week where the N32 calling convention is being partially handled by the O32 implementation.
> 
> Do you think it's best to fix that before or after this patch?
I've already started writing the necessary patches in order to fix this. I believe it would be better to submit this patch first, and deal with the N32/N64 problem later.

================
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:
----------------
dsanders wrote:
> > Why remove this signext? Likewise below.
> 
> Could you reply to this comment?
Sorry, I missed this. I wanted to minimize the number of changes that I had to do in the test cases. The patch that fixes the N32/N64 ABI problem rewrites a lot of these tests and adds the signext back.


http://reviews.llvm.org/D10970