[PATCH] D10970: [mips] Promote the result of SETCC nodes to GPR width.
Daniel Sanders via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 7 08:32:23 PDT 2015
dsanders requested changes to this revision.
dsanders added a comment.
This revision now requires changes to proceed.
Sorry for taking so long to get to this one.
It's generally looking good. Most of my comments are nits, minor issues, and questions.
================
Comment at: lib/Target/Mips/Disassembler/MipsDisassembler.cpp:1115
@@ +1114,3 @@
+ const void *Decoder) {
+ return DecodeFGRCC32RegisterClass(Inst, RegNo, Address, Decoder);
+}
----------------
This should probably be picking registers from FGRCC64 rather than FGRCC32.
================
Comment at: lib/Target/Mips/Mips32r6InstrInfo.td:764-837
@@ -801,41 +763,75 @@
// i32 selects
+multiclass SelectInt_Pats<ValueType RC, Instruction OROp, Instruction XORiOp,
+ Instruction SLTiOp, Instruction SLTiuOp,
+ Instruction SELEQZOp, Instruction SELNEZOp,
+ SDPatternOperator imm_type, ValueType VT> {
+// reg, immz
+def : MipsPat<(select (VT (seteq RC:$cond, immz)), RC:$t, RC:$f),
+ (OROp (SELEQZOp RC:$t, RC:$cond), (SELNEZOp RC:$f, RC:$cond))>;
+def : MipsPat<(select (VT (setne RC:$cond, immz)), RC:$t, RC:$f),
+ (OROp (SELNEZOp RC:$t, RC:$cond), (SELEQZOp RC:$f, RC:$cond))>;
+
+// reg, immZExt16[_64]
+def : MipsPat<(select (VT (seteq RC:$cond, imm_type:$imm)), RC:$t, RC:$f),
+ (OROp (SELEQZOp RC:$t, (XORiOp RC:$cond, imm_type:$imm)),
+ (SELNEZOp RC:$f, (XORiOp RC:$cond, imm_type:$imm)))>;
+def : MipsPat<(select (VT (setne RC:$cond, imm_type:$imm)), RC:$t, RC:$f),
+ (OROp (SELNEZOp RC:$t, (XORiOp RC:$cond, imm_type:$imm)),
+ (SELEQZOp RC:$f, (XORiOp RC:$cond, imm_type:$imm)))>;
+
+// reg, immSExt16Plus1
+def : MipsPat<(select (VT (setgt RC:$cond, immSExt16Plus1:$imm)), RC:$t, RC:$f),
+ (OROp (SELEQZOp RC:$t, (SLTiOp RC:$cond, (Plus1 imm:$imm))),
+ (SELNEZOp RC:$f, (SLTiOp RC:$cond, (Plus1 imm:$imm))))>;
+def : MipsPat<(select (VT (setugt RC:$cond, immSExt16Plus1:$imm)), RC:$t, RC:$f),
+ (OROp (SELEQZOp RC:$t, (SLTiuOp RC:$cond, (Plus1 imm:$imm))),
+ (SELNEZOp RC:$f, (SLTiuOp RC:$cond, (Plus1 imm:$imm))))>;
+
+def : MipsPat<(select (VT (seteq RC:$cond, immz)), RC:$t, immz),
+ (SELEQZOp RC:$t, RC:$cond)>;
+def : MipsPat<(select (VT (setne RC:$cond, immz)), immz, RC:$f),
+ (SELEQZOp RC:$f, RC:$cond)>;
+def : MipsPat<(select (VT (setne RC:$cond, immz)), RC:$t, immz),
+ (SELNEZOp RC:$t, RC:$cond)>;
+def : MipsPat<(select (VT (seteq RC:$cond, immz)), immz, RC:$f),
+ (SELNEZOp RC:$f, RC:$cond)>;
+}
+
+defm : SelectInt_Pats<i32, OR, XORi, SLTi, SLTiu, SELEQZ, SELNEZ,
+ immZExt16, i32>, ISA_MIPS32R6;
+
def : MipsPat<(select i32:$cond, i32:$t, i32:$f),
- (OR (SELNEZ i32:$t, i32:$cond), (SELEQZ i32:$f, i32:$cond))>,
- ISA_MIPS32R6;
-def : MipsPat<(select (i32 (seteq i32:$cond, immz)), i32:$t, i32:$f),
- (OR (SELEQZ i32:$t, i32:$cond), (SELNEZ i32:$f, i32:$cond))>,
- ISA_MIPS32R6;
-def : MipsPat<(select (i32 (setne i32:$cond, immz)), i32:$t, i32:$f),
- (OR (SELNEZ i32:$t, i32:$cond), (SELEQZ i32:$f, i32:$cond))>,
+ (OR (SELNEZ i32:$t, i32:$cond),
+ (SELEQZ i32:$f, i32:$cond))>,
ISA_MIPS32R6;
-def : MipsPat<(select (i32 (seteq i32:$cond, immZExt16:$imm)), i32:$t, i32:$f),
- (OR (SELEQZ i32:$t, (XORi i32:$cond, immZExt16:$imm)),
- (SELNEZ i32:$f, (XORi i32:$cond, immZExt16:$imm)))>,
- ISA_MIPS32R6;
-def : MipsPat<(select (i32 (setne i32:$cond, immZExt16:$imm)), i32:$t, i32:$f),
- (OR (SELNEZ i32:$t, (XORi i32:$cond, immZExt16:$imm)),
- (SELEQZ i32:$f, (XORi i32:$cond, immZExt16:$imm)))>,
- ISA_MIPS32R6;
-def : MipsPat<(select (i32 (setgt i32:$cond, immSExt16Plus1:$imm)), i32:$t,
- i32:$f),
- (OR (SELEQZ i32:$t, (SLTi i32:$cond, (Plus1 imm:$imm))),
- (SELNEZ i32:$f, (SLTi i32:$cond, (Plus1 imm:$imm))))>,
+def : MipsPat<(select i32:$cond, i32:$t, immz),
+ (SELNEZ i32:$t, i32:$cond)>,
ISA_MIPS32R6;
-def : MipsPat<(select (i32 (setugt i32:$cond, immSExt16Plus1:$imm)),
- i32:$t, i32:$f),
- (OR (SELEQZ i32:$t, (SLTiu i32:$cond, (Plus1 imm:$imm))),
- (SELNEZ i32:$f, (SLTiu i32:$cond, (Plus1 imm:$imm))))>,
+def : MipsPat<(select i32:$cond, immz, i32:$f),
+ (SELEQZ i32:$f, i32:$cond)>,
ISA_MIPS32R6;
-def : MipsPat<(select i32:$cond, i32:$t, immz),
- (SELNEZ i32:$t, i32:$cond)>, ISA_MIPS32R6;
-def : MipsPat<(select (i32 (setne i32:$cond, immz)), i32:$t, immz),
- (SELNEZ i32:$t, i32:$cond)>, ISA_MIPS32R6;
-def : MipsPat<(select (i32 (seteq i32:$cond, immz)), i32:$t, immz),
- (SELEQZ i32:$t, i32:$cond)>, ISA_MIPS32R6;
-def : MipsPat<(select i32:$cond, immz, i32:$f),
- (SELEQZ i32:$f, i32:$cond)>, ISA_MIPS32R6;
-def : MipsPat<(select (i32 (setne i32:$cond, immz)), immz, i32:$f),
- (SELEQZ i32:$f, i32:$cond)>, ISA_MIPS32R6;
-def : MipsPat<(select (i32 (seteq i32:$cond, immz)), immz, i32:$f),
- (SELNEZ i32:$f, i32:$cond)>, ISA_MIPS32R6;
+// comparisons supported via another comparison
+multiclass Cmp_Pats<ValueType VT, Instruction NOROp, Register ZEROReg> {
+def : MipsPat<(setone VT:$lhs, VT:$rhs),
+ (NOROp (!cast<Instruction>("CMP_UEQ_"#NAME) VT:$lhs, VT:$rhs), ZEROReg)>;
+def : MipsPat<(seto VT:$lhs, VT:$rhs),
+ (NOROp (!cast<Instruction>("CMP_UN_"#NAME) VT:$lhs, VT:$rhs), ZEROReg)>;
+def : MipsPat<(setne VT:$lhs, VT:$rhs),
+ (NOROp (!cast<Instruction>("CMP_EQ_"#NAME) VT:$lhs, VT:$rhs), ZEROReg)>;
+def : MipsPat<(setune VT:$lhs, VT:$rhs),
+ (NOROp (!cast<Instruction>("CMP_EQ_"#NAME) VT:$lhs, VT:$rhs), ZEROReg)>;
+def : MipsPat<(seteq VT:$lhs, VT:$rhs),
+ (!cast<Instruction>("CMP_EQ_"#NAME) VT:$lhs, VT:$rhs)>;
+def : MipsPat<(setgt VT:$lhs, VT:$rhs),
+ (!cast<Instruction>("CMP_LE_"#NAME) VT:$rhs, VT:$lhs)>;
+def : MipsPat<(setge VT:$lhs, VT:$rhs),
+ (!cast<Instruction>("CMP_LT_"#NAME) VT:$rhs, VT:$lhs)>;
+def : MipsPat<(setlt VT:$lhs, VT:$rhs),
+ (!cast<Instruction>("CMP_LT_"#NAME) VT:$lhs, VT:$rhs)>;
+def : MipsPat<(setle VT:$lhs, VT:$rhs),
+ (!cast<Instruction>("CMP_LE_"#NAME) VT:$lhs, VT:$rhs)>;
+}
+
+defm S : Cmp_Pats<f32, NOR, ZERO>, ISA_MIPS32R6;
+defm D : Cmp_Pats<f64, NOR, ZERO>, ISA_MIPS32R6;
----------------
Just to make the patch a bit more readable: Could you separate the change to the multiclass style into another patch?
Also, indentation and unnecessary reordering. (the contents of Cmp_Pats used to be at the start of this hunk)
================
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?
================
Comment at: lib/Target/Mips/Mips64r6InstrInfo.td:113-114
@@ -107,1 +112,4 @@
def SELNEZ64 : SELNEZ_ENC, SELNEZ64_DESC, ISA_MIPS32R6, GPR_64;
+ def SEL64_S : SEL_S_ENC, SEL64_S_DESC, ISA_MIPS32R6, GPR_64;
+ def SEL64_D : SEL_D_ENC, SEL64_D_DESC, ISA_MIPS32R6, GPR_64;
+ let isCodeGenOnly = 1 in {
----------------
ISA_MIPS32R6 and GPR_64 together is only possible for ISA_MIPS64R6.
I see that there are four pre-existing cases of ISA_MIPS32R6 in this file and they look like bugs to me.
================
Comment at: lib/Target/Mips/Mips64r6InstrInfo.td:115
@@ +114,3 @@
+ def SEL64_D : SEL_D_ENC, SEL64_D_DESC, ISA_MIPS32R6, GPR_64;
+ let isCodeGenOnly = 1 in {
+ defm S64: CMP_CC_M<FIELD_CMP_FORMAT_S, "s", FGRCC64Opnd, FGR32Opnd>, GPR_64;
----------------
Why codegen only?
================
Comment at: lib/Target/Mips/Mips64r6InstrInfo.td:117
@@ +116,3 @@
+ defm S64: CMP_CC_M<FIELD_CMP_FORMAT_S, "s", FGRCC64Opnd, FGR32Opnd>, GPR_64;
+ defm D64: CMP_CC_M<FIELD_CMP_FORMAT_S, "d", FGRCC64Opnd, FGR64Opnd>, GPR_64;
+ }
----------------
FIELD_CMP_FORMAT_S should be FIELD_CMP_FORMAT_D
================
Comment at: lib/Target/Mips/MipsCondMov.td:157-176
@@ +156,22 @@
+// reg, reg
+def : MipsPat<(select (iPTR (setge CRC:$lhs, CRC:$rhs)), DRC:$T, DRC:$F),
+ (MOVZInst DRC:$T, (SLTOp CRC:$lhs, CRC:$rhs), DRC:$F)>;
+def : MipsPat<(select (iPTR (setuge CRC:$lhs, CRC:$rhs)), DRC:$T, DRC:$F),
+ (MOVZInst DRC:$T, (SLTuOp CRC:$lhs, CRC:$rhs), DRC:$F)>;
+def : MipsPat<(select (iPTR (setle CRC:$lhs, CRC:$rhs)), DRC:$T, DRC:$F),
+ (MOVZInst DRC:$T, (SLTOp CRC:$rhs, CRC:$lhs), DRC:$F)>;
+def : MipsPat<(select (iPTR (setule CRC:$lhs, CRC:$rhs)), DRC:$T, DRC:$F),
+ (MOVZInst DRC:$T, (SLTuOp CRC:$rhs, CRC:$lhs), DRC:$F)>;
+
+// reg, imm
+def : MipsPat<(select (iPTR (setge CRC:$lhs, immSExt16:$rhs)), DRC:$T, DRC:$F),
+ (MOVZInst DRC:$T, (SLTiOp CRC:$lhs, immSExt16:$rhs), DRC:$F)>;
+def : MipsPat<(select (iPTR (setuge CRC:$lh, immSExt16:$rh)), DRC:$T, DRC:$F),
+ (MOVZInst DRC:$T, (SLTiuOp CRC:$lh, immSExt16:$rh), DRC:$F)>;
+def : MipsPat<
+ (select (iPTR (setgt CRC:$lhs, immSExt16Plus1:$rhs)), DRC:$T, DRC:$F),
+ (MOVZInst DRC:$T, (SLTiOp CRC:$lhs, (Plus1 imm:$rhs)), DRC:$F)>;
+def : MipsPat<
+ (select (iPTR (setugt CRC:$lhs, immSExt16Plus1:$rhs)), DRC:$T, DRC:$F),
+ (MOVZInst DRC:$T, (SLTiuOp CRC:$lhs, (Plus1 imm:$rhs)), DRC:$F)>;
+}
----------------
iPTR is i32 on N32 but mov[zn] is still a 64-bit move.
================
Comment at: lib/Target/Mips/MipsCondMov.td:179-180
@@ -204,1 +178,4 @@
+
+defm : MovzPats0<GPR32, GPR32, MOVZ_I_I,
+ SLT, SLTu, SLTi, SLTiu>,
INSN_MIPS4_32_NOT_32R6_64R6;
----------------
This can fit on one line. Similarly below
================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:640-641
@@ +639,4 @@
+ if (Diff == 1) {
+ if (Subtarget.isGP64bit() && False.getValueType() == MVT::i32)
+ SetCC = DAG.getNode(ISD::TRUNCATE, DL, MVT::i32, SetCC);
+
----------------
I think this should be more general. E.g.:
if (SetCC.getValueType().getSizeInBits() > False.getValueType().getSizeInBits())
SetCC = DAG.getNode(ISD::TRUNCATE, DL, False.getValueType, SetCC);
================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:656-657
@@ +655,4 @@
+
+ if (Subtarget.isGP64bit() && True.getValueType() == MVT::i32)
+ SetCC = DAG.getNode(ISD::TRUNCATE, DL, MVT::i32, SetCC);
+
----------------
Likewise
================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:1708-1709
@@ +1707,4 @@
+ Op.getValueType());
+ assert(((VT == MVT::i32) || (VT == MVT::i64)) &&
+ "Result type of SETCC is not i32 or i64.");
+
----------------
I don't see the reason for this assert at the moment. Is it a requirement of createCMovFP()? If so, shouldn't it be asserted inside that function?
================
Comment at: lib/Target/Mips/MipsRegisterInfo.td:394
@@ -393,2 +393,3 @@
// MIPS32r6/MIPS64r6 store FPU condition codes in normal FGR registers.
// This class allows us to represent this in codegen patterns.
+def FGRCC32 : RegisterClass<"Mips", [i32], 32, (sequence "F%u", 0, 31)>;
----------------
'This class allows' -> 'These classes allow'
================
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
}
----------------
Do you know why we still get this redundant sign extend? I was expecting it to disappear once the result type was i64.
================
Comment at: test/CodeGen/Mips/cmov.ll:469-472
@@ -484,6 +468,6 @@
; FIXME: The 32-bit versions of this test are too complicated to reasonably
; match at the moment. They do show some missing optimizations though
; such as:
; (movz $a, $b, (neg $c)) -> (movn $a, $b, $c)
----------------
Is this still true?
================
Comment at: test/CodeGen/Mips/countleading.ll:7
@@ -6,3 +6,3 @@
; RUN: llc -march=mips64el -mcpu=mips64r2 < %s | FileCheck -check-prefix=ALL -check-prefix=MIPS64-GT-R1 %s
-; R!N: llc -march=mips64el -mcpu=mips64r6 < %s | FileCheck -check-prefix=ALL -check-prefix=MIPS64-GT-R1 %s
+; RUN: llc -march=mips64el -mcpu=mips64r6 < %s | FileCheck -check-prefix=ALL -check-prefix=MIPS64-GT-R1 %s
----------------
Well spotted
================
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
----------------
I think you're missing the signext attribute on the result of the function.
Likewise below
================
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
----------------
This one is doubly-redundant. cmp.eq.s produces GPR-width 0 and -1 and we're also about to zero-extend 1-bit to GPR-width. I'm surprised the latter reason didn't remove it.
================
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.
http://reviews.llvm.org/D10970
More information about the llvm-commits
mailing list