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

[Ehsan Amiri via llvm-commits]

amehsan added inline comments.


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


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


================
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)
----------------
hfinkel wrote:
> 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.
> 
That's right. I will put your explanation in the code with some minor changes if needed.


https://reviews.llvm.org/D25991