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

[Hal Finkel via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1584
+
+  unsigned otherReg =
+    (CROp->getOperand(1).getReg() == CopyReg ?
----------------
s/otherReg/OtherReg/


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1585
+  unsigned otherReg =
+    (CROp->getOperand(1).getReg() == CopyReg ?
+                                     CROp->getOperand(2).getReg() :
----------------
What is the conditional here on CopyReg actually testing. How could it be zero (i.e. an invalid register) if hasOneUse(CopyReg) was true?


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1599
+  auto *ZeroFP = MRI->getUniqueVRegDef(CmpOp->getOperand(2).getReg());
+  if (ZeroFP->getOpcode() != PPC::XXLXORdpz &&
+      ZeroFP->getOpcode() != PPC::XXLXORspz)
----------------
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.



================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1631
   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;
----------------
No need for this white-space change.


https://reviews.llvm.org/D25991