[PATCH] D62629: [InstCombine] Update fptrunc (fneg x)) -> (fneg (fptrunc x) for unary FNeg

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally marked an inline comment as not done.
cameron.mcinally added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:1625
+    Value *InnerTrunc = Builder.CreateFPTrunc(X, Ty);
+    return BinaryOperator::CreateFNegFMF(InnerTrunc, cast<Instruction>(Op));
   }
----------------
cameron.mcinally wrote:
> craig.topper wrote:
> > If you started with a unary fneg, shouldn't we end up with a unary fneg?
> So this combine actually covers both unary and binary FNeg. `m_FNeg(X)` will match an `FSub(-0.0,X)`. The end goal is that both will end up producing an unary FNeg.
> 
> But, yeah, I see what you're saying. I could dispatch on the operator type for now, but it would be code to clean up later. I suppose the BinaryOperator will have to be switched to an UnaryOperator in the future anyway, so I'm not sure how much work I'm saving.
> 
> Just thinking aloud, I don't have a preference...
Thinking about this some more, you’re correct. It’s not safe to turn a unary FNeg into a binary FNeg. 

I was thinking about this as a short term solution, but we should play it safe in case this does not get updated. 

I’ll update the patch...



Repository:
  rL LLVM

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

https://reviews.llvm.org/D62629