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

[Hal Finkel via llvm-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/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.



================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1585
+  unsigned otherReg =
+    (CROp->getOperand(1).getReg() == CopyReg ?
+                                     CROp->getOperand(2).getReg() :
----------------
amehsan wrote:
> nemanjai wrote:
> > amehsan wrote:
> > > hfinkel wrote:
> > > > amehsan wrote:
> > > > > 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. 
> > > > 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)?
> > > > 
> > > 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 ?
> > 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.
> I just realized that the indentation, could cause it to be misinterpreted. I will check clang-format. 
Ah, indeed. I misread it. Thanks!


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1589
+
+  auto *MI = MRI->getUniqueVRegDef(otherReg);
+  if (MI->getOpcode() != PPC::COPY)
----------------
nemanjai wrote:
> 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?
No. We should check the return code. As in the function below we have:

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



https://reviews.llvm.org/D25991