[PATCH] D86871: [SelectionDAG] Let NSW/NUW flags be cleared by default in call to getNode().

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

Interesting.   I agree that the flags should always be reset here.  In fact, I'm questioning why we should have that "isDefined" logic in the first place.  It seems to me that any node with "undefined" flags can only be used correctly if any setting of the flags would be semantically correct -- but then we may just as well use the all-zero setting of the flags anyway.

However, doing that for the floating-point flags would break the current logic in SelectionDAGBuilder::visit, which does:

  if (SDNode *Node = getNodeForIRValue(&I)) {
    SDNodeFlags IncomingFlags;
    IncomingFlags.copyFMF(*FPMO);
    if (!Node->getFlags().isDefined())
      Node->setFlags(IncomingFlags);
    else
      Node->intersectFlagsWith(IncomingFlags);
  }

So it relies on nodes initially being created without defined flags, and then setting the FMF flags after the fact.

But it seems to me this logic is actually wrong: the Node returned from getNodeForIRValue may itself have multiple uses, some of which allow FMF and others do not.  So just blindly setting the flags here probably breaks in some cases.

Similarly, pretty much every other use of setFlags in SelectionDAG runs into the same issue.  Really, the only safe usage of flags is to specific them in the initial DAG.getNode call to begin with.  It seems this will require a significant refactoring.

However, as to the NUW/NSW flags specifically, your patch looks correct to me, but I'd appreciate comments from folks more familiar with the overall SelectionDAG logic as well.


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

https://reviews.llvm.org/D86871