[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.

In D86871#2252248 <https://reviews.llvm.org/D86871#2252248>, @spatel wrote:

> In D86871#2249833 <https://reviews.llvm.org/D86871#2249833>, @uweigand wrote:
>
>> Thanks for the links, @spatel !  It seems to me that the change in "D46854 <https://reviews.llvm.org/D46854> - changed code in SelectionDAGBuilder::visit()" is actually not correctly respecting the shared flags semantics.  The node returned from getNodeForIRValue, while implementing the translation of this IR, might at the same be be used elsewhere (due to DAG node merging).  If that other place does not allow FMF semantics, the flags in the DAG node can still be "undefined", and therefore the new code in "visit" will just replace the flags with a version appropriate only for this IR (which may allow FMF).
>>
>> Am I missing something here?   More generally, how can **any** after-the-fact setFlags ever be correct, given that the node might have already been merged?
>
> That does seem wrong given the Flags.isDefined() check sitting within intersectWith(). I'm not seeing any regression test failures if we remove that entirely. So let's do that instead of only pushing it below the nsw/nuw intersects?

I agree removing the isDefined() check within intersectWith() is a good idea, and if we can do it without regression, we should do so.  (And that will certainly fix the original problem Jonas was seeing.)

I still believe that even with that change most (all?) uses of setFlags are dangerous as they could introduce a flag into a shared node that may not be appropriate for all existing uses.  But maybe that can then be a separate discussion elsewhere.


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

https://reviews.llvm.org/D86871