From fe39edde27147d4645aff715d5a7630fa07fa885 Mon Sep 17 00:00:00 2001 From: Chris Lattner Date: Tue, 8 Jan 2008 06:46:30 +0000 Subject: [PATCH] Finally implement correct ordered comparisons for PPC, even though the code generated is not wonderful. This turns a miscompilation into a code quality bug (noted in the ppc readme). This fixes PR642, which is over 2 years old (!). Nate, please review this. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@45742 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Target/PowerPC/PPCISelDAGToDAG.cpp | 67 +++++++++++++++--------- lib/Target/PowerPC/README.txt | 29 ++++++++++ test/CodeGen/PowerPC/compare-fcmp-ord.ll | 27 ++++++++++ 3 files changed, 99 insertions(+), 24 deletions(-) create mode 100644 test/CodeGen/PowerPC/compare-fcmp-ord.ll diff --git a/lib/Target/PowerPC/PPCISelDAGToDAG.cpp b/lib/Target/PowerPC/PPCISelDAGToDAG.cpp index 6e8cebd979a..df1d9b5d25b 100644 --- a/lib/Target/PowerPC/PPCISelDAGToDAG.cpp +++ b/lib/Target/PowerPC/PPCISelDAGToDAG.cpp @@ -624,29 +624,34 @@ static PPC::Predicate getPredicateForSetCC(ISD::CondCode CC) { /// getCRIdxForSetCC - Return the index of the condition register field /// associated with the SetCC condition, and whether or not the field is /// treated as inverted. That is, lt = 0; ge = 0 inverted. -static unsigned getCRIdxForSetCC(ISD::CondCode CC, bool& Inv) { +/// +/// If this returns with Other != -1, then the returned comparison is an or of +/// two simpler comparisons. In this case, Invert is guaranteed to be false. +static unsigned getCRIdxForSetCC(ISD::CondCode CC, bool &Invert, int &Other) { + Invert = false; + Other = -1; switch (CC) { default: assert(0 && "Unknown condition!"); abort(); - case ISD::SETOLT: // FIXME: This is incorrect see PR642. - case ISD::SETULT: - case ISD::SETLT: Inv = false; return 0; - case ISD::SETOGE: // FIXME: This is incorrect see PR642. + case ISD::SETOLT: + case ISD::SETLT: return 0; // Bit #0 = SETOLT + case ISD::SETOGT: + case ISD::SETGT: return 1; // Bit #1 = SETOGT + case ISD::SETOEQ: + case ISD::SETEQ: return 2; // Bit #2 = SETOEQ + case ISD::SETUO: return 3; // Bit #3 = SETUO case ISD::SETUGE: - case ISD::SETGE: Inv = true; return 0; - case ISD::SETOGT: // FIXME: This is incorrect see PR642. - case ISD::SETUGT: - case ISD::SETGT: Inv = false; return 1; - case ISD::SETOLE: // FIXME: This is incorrect see PR642. + case ISD::SETGE: Invert = true; return 0; // !Bit #0 = SETUGE case ISD::SETULE: - case ISD::SETLE: Inv = true; return 1; - case ISD::SETOEQ: // FIXME: This is incorrect see PR642. - case ISD::SETUEQ: - case ISD::SETEQ: Inv = false; return 2; - case ISD::SETONE: // FIXME: This is incorrect see PR642. + case ISD::SETLE: Invert = true; return 1; // !Bit #1 = SETULE case ISD::SETUNE: - case ISD::SETNE: Inv = true; return 2; - case ISD::SETO: Inv = true; return 3; - case ISD::SETUO: Inv = false; return 3; + case ISD::SETNE: Invert = true; return 2; // !Bit #2 = SETUNE + case ISD::SETO: Invert = true; return 3; // !Bit #3 = SETO + case ISD::SETULT: Other = 0; return 3; // SETOLT | SETUO + case ISD::SETUGT: Other = 1; return 3; // SETOGT | SETUO + case ISD::SETUEQ: Other = 2; return 3; // SETOEQ | SETUO + case ISD::SETOGE: Other = 1; return 2; // SETOGT | SETOEQ + case ISD::SETOLE: Other = 0; return 2; // SETOLT | SETOEQ + case ISD::SETONE: Other = 0; return 1; // SETOLT | SETOGT } return 0; } @@ -726,7 +731,8 @@ SDNode *PPCDAGToDAGISel::SelectSETCC(SDOperand Op) { } bool Inv; - unsigned Idx = getCRIdxForSetCC(CC, Inv); + int OtherCondIdx; + unsigned Idx = getCRIdxForSetCC(CC, Inv, OtherCondIdx); SDOperand CCReg = SelectCC(N->getOperand(0), N->getOperand(1), CC); SDOperand IntCR; @@ -737,7 +743,7 @@ SDNode *PPCDAGToDAGISel::SelectSETCC(SDOperand Op) { CCReg = CurDAG->getCopyToReg(CurDAG->getEntryNode(), CR7Reg, CCReg, InFlag).getValue(1); - if (PPCSubTarget.isGigaProcessor()) + if (PPCSubTarget.isGigaProcessor() && OtherCondIdx == -1) IntCR = SDOperand(CurDAG->getTargetNode(PPC::MFOCRF, MVT::i32, CR7Reg, CCReg), 0); else @@ -745,13 +751,26 @@ SDNode *PPCDAGToDAGISel::SelectSETCC(SDOperand Op) { SDOperand Ops[] = { IntCR, getI32Imm((32-(3-Idx)) & 31), getI32Imm(31), getI32Imm(31) }; - if (!Inv) { + if (OtherCondIdx == -1 && !Inv) return CurDAG->SelectNodeTo(N, PPC::RLWINM, MVT::i32, Ops, 4); - } else { - SDOperand Tmp = - SDOperand(CurDAG->getTargetNode(PPC::RLWINM, MVT::i32, Ops, 4), 0); + + // Get the specified bit. + SDOperand Tmp = + SDOperand(CurDAG->getTargetNode(PPC::RLWINM, MVT::i32, Ops, 4), 0); + if (Inv) { + assert(OtherCondIdx == -1 && "Can't have split plus negation"); return CurDAG->SelectNodeTo(N, PPC::XORI, MVT::i32, Tmp, getI32Imm(1)); } + + // Otherwise, we have to turn an operation like SETONE -> SETOLT | SETOGT. + // We already got the bit for the first part of the comparison (e.g. SETULE). + + // Get the other bit of the comparison. + Ops[1] = getI32Imm((32-(3-OtherCondIdx)) & 31); + SDOperand OtherCond = + SDOperand(CurDAG->getTargetNode(PPC::RLWINM, MVT::i32, Ops, 4), 0); + + return CurDAG->SelectNodeTo(N, PPC::OR, MVT::i32, Tmp, OtherCond); } diff --git a/lib/Target/PowerPC/README.txt b/lib/Target/PowerPC/README.txt index 74bd15038ac..a6034e1fa9a 100644 --- a/lib/Target/PowerPC/README.txt +++ b/lib/Target/PowerPC/README.txt @@ -706,3 +706,32 @@ __Z11no_overflowjj: //===---------------------------------------------------------------------===// +We compile some FP comparisons into an mfcr with two rlwinms and an or. For +example: +#include +int test(double x, double y) { return islessequal(x, y);} +int test2(double x, double y) { return islessgreater(x, y);} +int test3(double x, double y) { return !islessequal(x, y);} + +Compiles into (all three are similar, but the bits differ): + +_test: + fcmpu cr7, f1, f2 + mfcr r2 + rlwinm r3, r2, 29, 31, 31 + rlwinm r2, r2, 31, 31, 31 + or r3, r2, r3 + blr + +GCC compiles this into: + + _test: + fcmpu cr7,f1,f2 + cror 30,28,30 + mfcr r3 + rlwinm r3,r3,31,1 + blr + +which is more efficient and can use mfocr. See PR642 for some more context. + +//===---------------------------------------------------------------------===// diff --git a/test/CodeGen/PowerPC/compare-fcmp-ord.ll b/test/CodeGen/PowerPC/compare-fcmp-ord.ll new file mode 100644 index 00000000000..b96efd17b2b --- /dev/null +++ b/test/CodeGen/PowerPC/compare-fcmp-ord.ll @@ -0,0 +1,27 @@ +; RUN: llvm-as < %s | llc -march=ppc32 | grep or | count 3 +; This should produce one 'or' or 'cror' instruction per function. + +target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:128:128" +target triple = "i686-apple-darwin8" + +define i32 @test(double %x, double %y) nounwind { +entry: + %tmp3 = fcmp ole double %x, %y ; [#uses=1] + %tmp345 = zext i1 %tmp3 to i32 ; [#uses=1] + ret i32 %tmp345 +} + +define i32 @test2(double %x, double %y) nounwind { +entry: + %tmp3 = fcmp one double %x, %y ; [#uses=1] + %tmp345 = zext i1 %tmp3 to i32 ; [#uses=1] + ret i32 %tmp345 +} + +define i32 @test3(double %x, double %y) nounwind { +entry: + %tmp3 = fcmp ugt double %x, %y ; [#uses=1] + %tmp34 = zext i1 %tmp3 to i32 ; [#uses=1] + ret i32 %tmp34 +} +