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

Daniel Sanders via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 14 07:50:40 PST 2015


dsanders added a comment.

I think we're nearly there. Most of these are either minor issues or nits.


================
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 {
----------------
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?

================
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));
 }
----------------
Could you tell me which case made this necessary?

================
Comment at: lib/Target/Mips/MicroMips64r6InstrInfo.td:125-127
@@ +124,5 @@
+def SEL_S_MM64R6 : StdMMR6Rel, SEL_S_MMR6_ENC, SEL_S_MM64R6_DESC,
+                        ISA_MICROMIPS64R6, HARDFLOAT;
+def SEL_D_MM64R6 : StdMMR6Rel, SEL_D_MMR6_ENC, SEL_D_MM64R6_DESC,
+                        ISA_MICROMIPS64R6, HARDFLOAT;
+}
----------------
Indentation

================
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;
+}
----------------
> This is because operands are still 32-bit instead of GPR-width, right?

Could you reply to this comment?

================
Comment at: lib/Target/Mips/Mips64r6InstrInfo.td:111
@@ +110,3 @@
+let AdditionalPredicates = [NotInMicroMips],
+    DecoderNamespace = "Mips32r6_64r6_GP64" in {
+  def SELEQZ64 : SELEQZ_ENC, SELEQZ64_DESC, ISA_MIPS64R6;
----------------
Naming nit: I think this should be 'Mips64r6' since these instruction definitions are not valid for Mips32r6.

================
Comment at: lib/Target/Mips/Mips64r6InstrInfo.td:116-117
@@ -108,1 +115,4 @@
+  def SEL64_D : SEL_D_ENC, SEL64_D_DESC, ISA_MIPS64R6, HARDFLOAT;
+  defm S64: CMP_CC_M<FIELD_CMP_FORMAT_S, "s", FGRCC64Opnd, FGR32Opnd>, ISA_MIPS64R6;
+  defm D64: CMP_CC_M<FIELD_CMP_FORMAT_D, "d", FGRCC64Opnd, FGR64Opnd>, ISA_MIPS64R6;
 }
----------------
80 cols?

================
Comment at: lib/Target/Mips/MipsCondMov.td:216-222
@@ -228,8 +215,9 @@
 
-defm : MovnPats<GPR32, GPR64, MOVN_I_I64, XOR>, INSN_MIPS4_32_NOT_32R6_64R6,
-       GPR_64;
-defm : MovnPats<GPR64, GPR32, MOVN_I64_I, XOR64>, INSN_MIPS4_32_NOT_32R6_64R6,
-       GPR_64;
-defm : MovnPats<GPR64, GPR64, MOVN_I64_I64, XOR64>, INSN_MIPS4_32_NOT_32R6_64R6,
-       GPR_64;
+multiclass MovzPats1<RegisterClass CRC, RegisterClass DRC,
+                     Instruction MOVZInst, Instruction XOROp> {
+def : MipsPat<(select (iPTR (seteq CRC:$lhs, CRC:$rhs)), DRC:$T, DRC:$F),
+              (MOVZInst DRC:$T, (XOROp CRC:$lhs, CRC:$rhs), DRC:$F)>;
+def : MipsPat<(select (iPTR (seteq CRC:$lhs, 0)), DRC:$T, DRC:$F),
+              (MOVZInst DRC:$T, CRC:$lhs, DRC:$F)>;
+}
 
----------------
This isn't quite right. iPTR is i32 for N32 but comparisons should produce i64.

================
Comment at: lib/Target/Mips/MipsCondMov.td:244-246
@@ +243,5 @@
+                     Instruction MOVZInst, Instruction XORiOp> {
+def : MipsPat<
+    (select (iPTR (seteq CRC:$lhs, immZExt16:$uimm16)), DRC:$T, DRC:$F),
+    (MOVZInst DRC:$T, (XORiOp CRC:$lhs, immZExt16:$uimm16), DRC:$F)>;
+}
----------------
This isn't quite right. iPTR is i32 for N32 but comparisons should produce i64.

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:268-269
@@ -267,3 +267,4 @@
   // which is used implicitly by brcond and select operations.
-  AddPromotedToType(ISD::SETCC, MVT::i1, MVT::i32);
+  AddPromotedToType(ISD::SETCC, MVT::i1,
+                    Subtarget.isGP32bit() ? MVT::i32 : MVT::i64);
 
----------------
It ought to be based on the ABI rather than the architecture. Can you use MipsABIInfo::AreGprs64bit()?

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:469
@@ -468,1 +468,3 @@
+
+  return Subtarget.isGP32bit() ? MVT::i32 : MVT::i64;
 }
----------------
It ought to be based on the ABI rather than the architecture. Can you use MipsABIInfo::AreGprs64bit()?

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:3973
@@ -3929,3 +3972,3 @@
       .addReg(MI->getOperand(1).getReg())
-      .addReg(Mips::ZERO)
+      .addReg(Subtarget.isGP32bit() ? Mips::ZERO : Mips::ZERO_64)
       .addMBB(sinkMBB);
----------------
It ought to be based on the ABI rather than the architecture. Can you use MipsABIInfo::AreGprs64bit()?

================
Comment at: lib/Target/Mips/MipsSEISelLowering.h:71-72
@@ -70,2 +70,4 @@
 
+    SDValue lowerOverflowRes(SDValue Op, SelectionDAG &DAG) const;
+
     SDValue lowerINTRINSIC_WO_CHAIN(SDValue Op, SelectionDAG &DAG) const;
----------------
Did you mean to include this? There's no implementation.

================
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
 }
----------------
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?

================
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:
----------------
> Why remove this signext? Likewise below.

Could you reply to this comment?


http://reviews.llvm.org/D10970





More information about the llvm-commits mailing list