[PATCH] D67553: [InstSimplify] Match 1.0 and 0.0 for both operands in SimplifyFMAMul

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.

I'm fine with this approach.



================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4561
                               const SimplifyQuery &Q, unsigned MaxRecurse) {
   // fmul X, 1.0 ==> X
   if (match(Op1, m_FPOne()))
----------------
reames wrote:
> reames wrote:
> > fhahn wrote:
> > > lebedev.ri wrote:
> > > > I'm wondering if 
> > > > ```
> > > > {
> > > > if(isa<Constant>(Op0)) std::swap(Op0, Op1);
> > > > ```
> > > > would be simpler?
> > > I initially also used to swap the operands, but we'd also have to check if it's a 1.1 or 0.0 constant I think, to handle `fmul 10.0, 0.0`. That's not a problem with SimplifyFMulInst, as constant folding would cover this case.
> > I believe that fmul is not necessarily commutative (for nan inputs).  Given that, swap would be correct for this case, but dangerous in general.
> Er, maybe I'm wrong here.  At least the accessors in Instruction.h seems to think so.  
I'm pretty sure fadd/fmul are commutative (`X * Y == Y * X`), but not associative (`(X * Y) * Z != (X * Z) * Y`).
So accessors are likely wrong. Or many more code is wrong elsewhere.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67553/new/

https://reviews.llvm.org/D67553