[PPC] Peephole to remove extra fcmp that checks for NaN

[amehsan]

Summary

The functionality is explained in a comment in the patch. So I do not repeat that here.

This problem is exposed after a patch that I recently committed to generate +0.0 using xor, instead of loading it from constant area. Currently this is a targetted fix for this particular pattern. I tried a little to create other testcases with a similar problem, but that was not successful. If you have ideas for generalizing the patch, I can update it.

[amehsan]

test/CodeGen/PowerPC/fast-isel-fcmp-nan.ll

4–7

I think my comment here is not quite accurate. I will update it to something like this:

; Some tests need to check two bits of the CR field, so we check for a
; pattern that indicates both CR bits are checked. Otherwise we look for a cmp followed
; by a jump. For O2 tests, we need to make sure that only one comparison is done.

[hfinkel]

lib/Target/PowerPC/PPCInstrInfo.cpp

1584

s/otherReg/OtherReg/

1585

What is the conditional here on CopyReg actually testing. How could it be zero (i.e. an invalid register) if hasOneUse(CopyReg) was true?

1599

This makes sense, but we should add a comment explaining why. The point here is that we're trying to figure out whether (vreg2 < 0 or isnan(vreg)). When we use FCMPU, the un bit will be set if either of the operands is nan. So this is only semantically valid when we know that the other operand of the compare is not nan. We do know this when we know it is zero.

1631

No need for this white-space change.

[amehsan]

lib/Target/PowerPC/PPCInstrInfo.cpp

1584

Will change

1585

The CROp has two source operands (Operands 1 and 2). If Operand(1) is the same Reg that was defined by Copy insn we just visited (CopyReg), then we look at Operand (2) and go to its definition. Otherwise, Operand(2) will be CopyReg and we should start from Operand(1) and go to its definition, etc.

1599

That's right. I will put your explanation in the code with some minor changes if needed.

[echristo]

lib/Target/PowerPC/PPCInstrInfo.cpp

1543

Actually, go ahead and put this in the commit message as well. That way there's an easier record of it there too. More detailed commit messages are often goodness.

[amehsan]

lib/Target/PowerPC/PPCInstrInfo.cpp

1543

Sure

[amehsan]

lib/Target/PowerPC/PPCInstrInfo.cpp

1631

OK. Will undo it.

[hfinkel]

lib/Target/PowerPC/PPCInstrInfo.cpp

1585

I still don't understand. CopyReg is defined above as:

unsigned CopyReg = CopyMI->getOperand(0).getReg();
if (!MRI->hasOneUse(CopyReg))
  return false;

under what conditions is CopyReg zero (i.e. an invalid register)?

[amehsan]

lib/Target/PowerPC/PPCInstrInfo.cpp

1585

I am probably missing something. CopyReg is the register that contains the output of COPY instruction. It should not be zero. Maybe there are edge cases that I am not aware of ?

[nemanjai]

lib/Target/PowerPC/PPCInstrInfo.cpp

1567

s/sub/Sub?

1585

But the condition is not for whether CopyReg is zero but rather whether it is the same register as the first operand of the CROR/CRNOR.

1589

I haven't thought about this enough to decipher whether this is possible so I'd rather ask...
Are all of these getUniqueVRegDef calls guaranteed to succeed? Namely, is it impossible that any of the arguments are physical registers at this point?

1619

This may be a naive question since I don't really know under which conditions we emit XSCMPUDP vs. FCMPUD, but can this not be done with XSCMPUDP as well? Or perhaps there is no need for this.

[amehsan]

lib/Target/PowerPC/PPCInstrInfo.cpp

1585

I just realized that the indentation, could cause it to be misinterpreted. I will check clang-format.

1619

That's a good question. The problem is PPCInstrInfo::analyzeCompare() does not return true for FCMPUD so from Peephole opt point of view XSCMPUD is not a compare opcode. Since there might be some finer points that has to be take into account, I opened a PR for this, rather than fixing it here.

[amehsan]

lib/Target/PowerPC/PPCInstrInfo.cpp

1619

Typo in the comment: PPCInstrInfo::analyzeCompare() does not return true for XSCMPUDP

[amehsan]

There has been many comments here, but only a couple of minor issues has to be addressed. (check indentation, remove whitespace, change some variable names). If you don't mind to approve it, I will fix those before committing.

[hfinkel]

In D25991#580723, @amehsan wrote:

There has been many comments here, but only a couple of minor issues has to be addressed. (check indentation, remove whitespace, change some variable names). If you don't mind to approve it, I will fix those before committing.

Yes, with the comments addressed, this LGTM.

lib/Target/PowerPC/PPCInstrInfo.cpp

1585

Ah, indeed. I misread it. Thanks!

1589

No. We should check the return code. As in the function below we have:

MachineInstr *MI = MRI->getUniqueVRegDef(SrcReg);
if (!MI) return false;

[amehsan]

lib/Target/PowerPC/PPCInstrInfo.cpp

1589

Right. I even tested this, but then I forgot to check the return value.

[amehsan]

This patch, as is, is incorrect and requires some changes to proceed. Abandoning.

lib/Target/PowerPC/PPCInstrInfo.h

public:
 
   bool isPredicable(MachineInstr &MI) const override;
 
   // Comparison optimization.
 
   bool analyzeCompare(const MachineInstr &MI, unsigned &SrcReg,
                       unsigned &SrcReg2, int &Mask, int &Value) const override;
 
+  bool optimizeFPCmpInstr(MachineInstr &CmpInstr, unsigned Src,
+                          unsigned Src2, int Mask, int Value,
+                          const MachineRegisterInfo *MRI) const;
+
   bool optimizeCompareInstr(MachineInstr &CmpInstr, unsigned SrcReg,
                             unsigned SrcReg2, int Mask, int Value,
                             const MachineRegisterInfo *MRI) const override;
 
   /// GetInstSize - Return the number of bytes of code the specified
   /// instruction may be.  This returns the maximum number of bytes.
   ///
   unsigned getInstSizeInBytes(const MachineInstr &MI) const override;

lib/Target/PowerPC/PPCInstrInfo.cpp

bool PPCInstrInfo::analyzeCompare(const MachineInstr &MI, unsigned &SrcReg,
   case PPC::FCMPUS:
   case PPC::FCMPUD:
     SrcReg = MI.getOperand(1).getReg();
     SrcReg2 = MI.getOperand(2).getReg();
     return true;
   }
 }
 
+bool PPCInstrInfo::optimizeFPCmpInstr(MachineInstr &CmpInstr, unsigned Src,
+                                      unsigned Src2, int Mask, int Value,
+                                      const MachineRegisterInfo *MRI) const {
+  // We want to remove the first fcmpu in patterns like this
+  //      fcmpu 0, 1, 1
+  //      fcmpu 1, 1, 0
+  //      cror 20, 4, 3
+  //      bc 4, 20, .LBB0_2
+  // Here we look for a pattern like this
+  //  %vreg2<def> = XXLXORdpz; F8RC:%vreg2
+  //  %vreg3<def> = FCMPUD %vreg0, %vreg2;
+  //  %vreg4<def> = COPY %vreg3:sub_lt;
+  //  %vreg5<def> = FCMPUD %vreg0, %vreg0;
+  //  %vreg6<def> = COPY %vreg5:sub_un; <-----
+  //  %vreg7<def> = CROR %vreg4, %vreg6;
+  // Then we replace vreg5 with vreg3 in the insn marked
+
+  if (Src != Src2)
+    return false;
+
+  unsigned CRReg = CmpInstr.getOperand(0).getReg();
+
+  if (!MRI->hasOneUse(CRReg))
+    return false;
+
+  // Op pointers to a MachineOperand which is the only use of CRReg
+  MachineRegisterInfo::use_iterator Op = MRI->use_begin(CRReg);
+  unsigned sub = Op->getSubReg();
+  if (sub != PPC::sub_un)
+    return false;
+
+  auto *CopyMI = Op->getParent();
+  if (CopyMI->getOpcode() != PPC::COPY)
+    return false;
+
+  unsigned CopyReg = CopyMI->getOperand(0).getReg();
+  if (!MRI->hasOneUse(CopyReg))
+    return false;
+
+  MachineRegisterInfo::use_iterator CopyUse = MRI->use_begin(CopyReg);
+  auto *CROp = CopyUse->getParent();
+  if (CROp->getOpcode() != PPC::CROR && CROp->getOpcode() != PPC::CRNOR)
+    return false;
+
+  unsigned otherReg =
+    (CROp->getOperand(1).getReg() == CopyReg ?
+                                     CROp->getOperand(2).getReg() :
+                                     CROp->getOperand(1).getReg());
+
+  auto *MI = MRI->getUniqueVRegDef(otherReg);
+  if (MI->getOpcode() != PPC::COPY)
+    return false;
+
+  auto CopiedReg = MI->getOperand(1).getReg();
+  auto *CmpOp = MRI->getUniqueVRegDef(CopiedReg);
+  if (CmpOp->getOpcode() != CmpInstr.getOpcode())
+    return false;
+
+  auto *ZeroFP = MRI->getUniqueVRegDef(CmpOp->getOperand(2).getReg());
+  if (ZeroFP->getOpcode() != PPC::XXLXORdpz &&
+      ZeroFP->getOpcode() != PPC::XXLXORspz)
+    return false;
+
+  Op->setReg(CmpOp->getOperand(0).getReg());
+
+  return true;
+}
+
 bool PPCInstrInfo::optimizeCompareInstr(MachineInstr &CmpInstr, unsigned SrcReg,
                                         unsigned SrcReg2, int Mask, int Value,
                                         const MachineRegisterInfo *MRI) const {
   if (DisableCmpOpt)
     return false;
 
   int OpC = CmpInstr.getOpcode();
   unsigned CRReg = CmpInstr.getOperand(0).getReg();
 
   // FP record forms set CR1 based on the execption status bits, not a
   // comparison with zero.
   if (OpC == PPC::FCMPUS || OpC == PPC::FCMPUD)
-    return false;
+    return optimizeFPCmpInstr(CmpInstr, SrcReg, SrcReg2, Mask, Value, MRI);
 
   // The record forms set the condition register based on a signed comparison
   // with zero (so says the ISA manual). This is not as straightforward as it
   // seems, however, because this is always a 64-bit comparison on PPC64, even
   // for instructions that are 32-bit in nature (like slw for example).
   // So, on PPC32, for unsigned comparisons, we can use the record forms only
   // for equality checks (as those don't depend on the sign). On PPC64,
   // we are restricted to equality for unsigned 64-bit comparisons and for
   // signed 32-bit comparisons the applicability is more restricted.
   bool isPPC64 = Subtarget.isPPC64();
-  bool is32BitSignedCompare   = OpC ==  PPC::CMPWI || OpC == PPC::CMPW;
+  bool is32BitSignedCompare   = OpC == PPC::CMPWI || OpC == PPC::CMPW;
   bool is32BitUnsignedCompare = OpC == PPC::CMPLWI || OpC == PPC::CMPLW;
   bool is64BitUnsignedCompare = OpC == PPC::CMPLDI || OpC == PPC::CMPLD;
 
   // Get the unique definition of SrcReg.
   MachineInstr *MI = MRI->getUniqueVRegDef(SrcReg);
   if (!MI) return false;
   int MIOpC = MI->getOpcode();
 

test/CodeGen/PowerPC/fast-isel-fcmp-nan.ll

 ; RUN: llc -mtriple powerpc64le-unknown-linux-gnu -fast-isel -O0 < %s | FileCheck %s
+; RUN: llc -mtriple powerpc64le-unknown-linux-gnu -O2 < %s | FileCheck --check-prefix=CHECK-O2 %s
+
+; Tests that need to check for NaN need to look at two CR bits, so we check for a
+; pattern that indicates both CR bits are checked. Otherwise we look for a cmp followed
+; by a jump. For O2 tests, we need to make sure that only one comparison is done.
 
 define i1 @TestULT(double %t0) {
 ; CHECK-LABEL: TestULT:
-; CHECK: xscmpudp
-; CHECK: blr
+; CHECK: mfocrf [[REG1:[0-9]*]]
+; CHECK: mfocrf [[REG2:[0-9]*]]
+; CHECK: or {{[0-9]*}}, [[REG1]], [[REG2]]
+; CHECK: blr
+; CHECK-O2: xxlxor
+; CHECK-O2-NEXT: fcmpu
+; CHECK-O2-NEXT: cror
+; CHECK-O2-NEXT: bc
+; CHECK-O2: blr
 entry:
   %t1 = fcmp ult double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestULE(double %t0) {
 ; CHECK-LABEL: TestULE:
-; CHECK: fcmpu
-; CHECK-NEXT: ble
+; CHECK: fcmpu [[REG3:[0-9]*]]
+; CHECK-NEXT: ble [[REG3]]
 ; CHECK: blr
+; CHECK-O2: xxlxor
+; CHECK-O2-NEXT: xscmpudp
+; CHECK-O2-NEXT: bgt
+; CHECK-O2: blr
 entry:
   %t1 = fcmp ule double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestUNE(double %t0) {
 ; CHECK-LABEL: TestUNE:
-; CHECK: fcmpu
-; CHECK-NEXT: bne
+; CHECK: fcmpu [[REG4:[0-9]*]]
+; CHECK-NEXT: bne [[REG4]]
 ; CHECK: blr
+; CHECK-O2: xxlxor
+; CHECK-O2-NEXT: xscmpudp
+; CHECK-O2-NEXT: beq
+; CHECK-O2: blr
 entry:
   %t1 = fcmp une double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestUEQ(double %t0) {
 ; CHECK-LABEL: TestUEQ:
-; CHECK: xscmpudp
-; CHECK: blr
+; CHECK: mfocrf [[REG5:[0-9]*]]
+; CHECK: mfocrf [[REG6:[0-9]*]]
+; CHECK: or {{[0-9]*}}, [[REG5]], [[REG6]]
+; CHECK: blr
+; CHECK-O2: xxlxor
+; CHECK-O2-NEXT: fcmpu
+; CHECK-O2-NEXT: cror
+; CHECK-O2-NEXT: bc
+; CHECK-O2: blr
+
 entry:
   %t1 = fcmp ueq double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestUGT(double %t0) {
 ; CHECK-LABEL: TestUGT:
-; CHECK: xscmpudp
-; CHECK: blr
+; CHECK: mfocrf [[REG7:[0-9]*]]
+; CHECK: mfocrf [[REG8:[0-9]*]]
+; CHECK: or {{[0-9]*}}, [[REG7]], [[REG8]]
+; CHECK: blr
+; CHECK-O2: xxlxor
+; CHECK-O2-NEXT: fcmpu
+; CHECK-O2-NEXT: cror
+; CHECK-O2-NEXT: bc
+; CHECK-O2: blr
 entry:
   %t1 = fcmp ugt double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestUGE(double %t0) {
 ; CHECK-LABEL: TestUGE:
-; CHECK: fcmpu
-; CHECK-NEXT: bge
+; CHECK: fcmpu [[REG13:[0-9]*]]
+; CHECK-NEXT: bge [[REG13]]
 ; CHECK: blr
+; CHECK-O2: xxlxor
+; CHECK-O2-NEXT: xscmpudp
+; CHECK-O2-NEXT: blt
+; CHECK-O2: blr
 entry:
   %t1 = fcmp uge double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestOLT(double %t0) {
 ; CHECK-LABEL: TestOLT:
-; CHECK: fcmpu
-; CHECK-NEXT: blt
+; CHECK: fcmpu [[REG9:[0-9]*]]
+; CHECK-NEXT: blt [[REG9]]
 ; CHECK: blr
+; CHECK-O2: xxlxor
+; CHECK-O2-NEXT: xscmpudp
+; CHECK-O2-NEXT: bge
+; CHECK-O2: blr
 entry:
   %t1 = fcmp olt double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestOLE(double %t0) {
 ; CHECK-LABEL: TestOLE:
 ; CHECK: xscmpudp
-; CHECK: blr
+; CHECK: mfocrf
+; CHECK: rlwinm
+; CHECK: xori
+; CHECK: xscmpudp
+; CHECK: mfocrf
+; CHECK: clrlwi
+; CHECK: xori
+; CHECK: and
+; CHECK: blr
+; CHECK-O2: xxlxor
+; CHECK-O2-NEXT: fcmpu
+; CHECK-O2-NEXT: crnor
+; CHECK-O2-NEXT: bc
+; CHECK-O2: blr
 entry:
   %t1 = fcmp ole double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestONE(double %t0) {
 ; CHECK-LABEL: TestONE:
 ; CHECK: xscmpudp
-; CHECK: blr
+; CHECK: mfocrf
+; CHECK: rlwinm
+; CHECK: xori
+; CHECK: xscmpudp
+; CHECK: mfocrf
+; CHECK: clrlwi
+; CHECK: xori
+; CHECK: and
+; CHECK: blr
+; CHECK-O2: xxlxor
+; CHECK-O2-NEXT: fcmpu
+; CHECK-O2-NEXT: crnor
+; CHECK-O2-NEXT: bc
+; CHECK-O2: blr
 entry:
   %t1 = fcmp one double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestOEQ(double %t0) {
 ; CHECK-LABEL: TestOEQ:
-; CHECK: fcmpu
-; CHECK-NEXT: beq
+; CHECK: fcmpu [[REG11:[0-9]*]]
+; CHECK-NEXT: beq [[REG11]]
 ; CHECK: blr
+; CHECK-O2: xxlxor
+; CHECK-O2-NEXT: xscmpudp
+; CHECK-O2-NEXT: bne
+; CHECK-O2: blr
 entry:
   %t1 = fcmp oeq double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestOGT(double %t0) {
 ; CHECK-LABEL: TestOGT:
-; CHECK: fcmpu
-; CHECK-NEXT: bgt
+; CHECK: fcmpu [[REG12:[0-9]*]]
+; CHECK-NEXT: bgt [[REG12]]
 ; CHECK: blr
+; CHECK-O2: xxlxor
+; CHECK-O2-NEXT: xscmpudp
+; CHECK-O2-NEXT: ble
+; CHECK-O2: blr
 entry:
   %t1 = fcmp ogt double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }
 
 define i1 @TestOGE(double %t0) {
 ; CHECK-LABEL: TestOGE:
 ; CHECK: xscmpudp
-; CHECK: blr
+; CHECK: mfocrf
+; CHECK: rlwinm
+; CHECK: xori
+; CHECK: xscmpudp
+; CHECK: mfocrf
+; CHECK: clrlwi
+; CHECK: xori
+; CHECK: and
+; CHECK: blr
+; CHECK-O2: xxlxor
+; CHECK-O2-NEXT: fcmpu
+; CHECK-O2-NEXT: crnor
+; CHECK-O2-NEXT: bc
+; CHECK-O2: blr
 entry:
   %t1 = fcmp oge double %t0, 0.000000e+00
   br i1 %t1, label %good, label %bad
 
 bad:
   ret i1 false
 
 good:
   ret i1 true
 }