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

Sanjay Patel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 2 08:19:59 PDT 2020


spatel added a comment.

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?



================
Comment at: llvm/test/CodeGen/SystemZ/int-cmp-60.ll:7-10
+; CHECK: # After Instruction Selection:
+; CHECK-LABEL: bb.0.entry:
+; CHECK: = AFIMux
+; CHECK-NOT: = nuw nsw AFIMux
----------------
I think it would be less fragile to check the complete/correct asm for this test instead of using 'CHECK-NOT' on debug output.
If I'm seeing that correctly, we should get a "locrnhe" in the result if we fixed the bug.


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

https://reviews.llvm.org/D86871



More information about the llvm-commits mailing list