[PATCH] D28675: [DAGCombine] require UnsafeFPMath for re-association of addition

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D28675#659006, @nhaehnle wrote:

> @arsenm, unfortunately I can't add flag-based tests, since as @hfinkel points out, you can't actually get the flag on an FMA or FMAD in the DAG. @hfinkel, the idea was to put the check in already anyway, since by the discussion with @spatel, the plan is to add FMF for all nodes eventually. That seems reasonable to me.


But you can't because they'll crash:

const SDNodeFlags *SDNode::getFlags() const {

  if (auto *FlagsNode = dyn_cast<BinaryWithFlagsSDNode>(this))
    return &FlagsNode->Flags;
  return nullptr;

}

so if you actually reach that code, right now, when those nodes don't support flags, then you'll get the nullptr return here on which you're calling a member function. In any case, I'd leave them out for now. Someone needs to look at this again anyway to fix the FIXMEs, and then we'll add the code when it is testable.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8134
+        (Options.UnsafeFPMath || (N->getFlags()->hasUnsafeAlgebra() &&
+         N0.getNode()->getFlags()->hasUnsafeAlgebra()))) {
       return DAG.getNode(PreferredFusedOpcode, SL, VT,
----------------
nhaehnle wrote:
> hfinkel wrote:
> > arsenm wrote:
> > > I don't think you need the getNode here (and same for the rest
> > Why are you checking N0->getFlags()->hasUnsafeAlgebra() if N0 is a FMA and the comment says they don't support FMFs?
> > 
> I double-checked, SDValue doesn't have getFlags().
> I double-checked, SDValue doesn't have getFlags().

The point is that SDValue has:

  SDNode *getNode() const { return Node; }

and also:

  inline SDNode *operator->() const { return Node; }

so the -> operator is just like getNode().



https://reviews.llvm.org/D28675