[PATCH] D85548: Allow fneg + strict_fadd -> strict_fsub in DAGCombiner

[Andrew J Wock via Phabricator via llvm-commits]

ajwock added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12824
+  // fold (strict_fadd A, (fneg B)) -> (strict_fsub A, B)
+  if (!LegalOperations || TLI.isOperationLegalOrCustom(ISD::STRICT_FSUB, VT))
+    if (SDValue NegN1 = TLI.getCheaperNegatedExpression(
----------------
craig.topper wrote:
> arsenm wrote:
> > kpn wrote:
> > > arsenm wrote:
> > > > There is some strange legality predicate you are supposed to use for the strict cases, although I don't fully understand it
> > > Where is this predicate? Is there an existing case that can be modeled after?
> > > 
> > > My thinking is that this transform is probably safe because the FNeg will never trap and it will never round so it should therefore be safe to fold it into the add/sub. But I'm no expert on floating-point I admit.
> > This is totally valid. But for some reason, we have getStrictFPOperationAction which differs from checking the regular legalization action on the STRICT_ opcode
> I think getStrictFPOperationAction works along with the code in SelectionDAGISel that mutates strict nodes to non-strict nodes for targets that haven't done the work. It was intended to query whether the target will be able to handle the mutated node. It should go away when all targets have done the work.
> 
> Since I think we're now blocking clang from using strict fp on targets that haven't done the work. Then we probably can just use the normal legal operation check.
As far as I can tell, that comes into play later on as craig topper has stated.  I don't think there's any way for that to apply during combination.  Here we have to rely on operations that we can be sure are legal.  As long as this operation is legal according to the standard (I'm also no expert, but I'm pretty sure based on my interpretation) exceptions should still occur as the original code prescribes, and in the same order as such.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85548