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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa created this revision.
jonpa added reviewers: spatel, aemerson, mcberg2017, uweigand.
Herald added subscribers: steven.zhang, JDevlieghere.
Herald added a project: LLVM.
jonpa requested review of this revision.

Recently a new optimization was added to the DAGCombiner which exposed a problem with SDNode memoization (see bug report at https://bugs.llvm.org/show_bug.cgi?id=47092). The problem was that an existing ISD::ADD node node with NSW/NUW flags set was reused when the DAGCombiner produced an identical node except without any guarantee for no overflow. This led to false NSW/NUW flags on the resulting SystemZ MachineInstr, and since SystemZElimCompare.cpp trusts these flags when eliminating a compare with 0, wrong code resulted.

This patch changes the default behavior in these cases to clear any NSW/NUW flags in case they are not set by the caller of getNode(). This seems safer since otherwise wrong-code might result any time an optimization forgets to explicitly clear these flags.

This doesn't cause any test failures, and changes just one locr instruction on SPEC (cse.s) from locrlh to locrne (in that case an SRK instruction had an NSW flag which it seems it perhaps should not have...)

I am not sure about the FP flags so I left them out of this change, but maybe one or more of them also should be conservatively cleared by default?


https://reviews.llvm.org/D86871

Files:
  llvm/include/llvm/CodeGen/SelectionDAGNodes.h
  llvm/test/CodeGen/SystemZ/int-cmp-60.ll


Index: llvm/test/CodeGen/SystemZ/int-cmp-60.ll
===================================================================
--- /dev/null
+++ llvm/test/CodeGen/SystemZ/int-cmp-60.ll
@@ -0,0 +1,25 @@
+; RUN: llc < %s -mtriple=s390x-linux-gnu -mcpu=z14 -print-after-isel 2>&1 | FileCheck %s
+; REQUIRES: asserts
+;
+; Test that DAGCombiner properly clears the NUW/NSW flags on the memoized add
+; node.
+
+; CHECK: # After Instruction Selection:
+; CHECK-LABEL: bb.0.entry:
+; CHECK: = AFIMux
+; CHECK-NOT: = nuw nsw AFIMux
+
+define void @fun(i64* %Src, i32* %Dst) {
+entry:
+  %0 = load i64, i64* %Src, align 8
+  %1 = trunc i64 %0 to i32
+  %conv = and i32 %1, 1303940520
+  %xor11.i = or i32 %conv, -2147483648
+  %xor2.i = add i32 %xor11.i, -519348290
+  %cmp.i = icmp slt i32 %xor2.i, 0
+  %sub3.i = add nuw nsw i32 %conv, 1628135358
+  %cond.i = select i1 %cmp.i, i32 %conv, i32 %sub3.i
+  store i32 %cond.i, i32* %Dst
+  ret void
+}
+
Index: llvm/include/llvm/CodeGen/SelectionDAGNodes.h
===================================================================
--- llvm/include/llvm/CodeGen/SelectionDAGNodes.h
+++ llvm/include/llvm/CodeGen/SelectionDAGNodes.h
@@ -359,7 +359,8 @@
 private:
   // This bit is used to determine if the flags are in a defined state.
   // Flag bits can only be masked out during intersection if the masking flags
-  // are defined.
+  // are defined, but in that case the NoUnsignedWrap/NoSignedWrap flags are
+  // cleared.
   bool AnyDefined : 1;
 
   bool NoUnsignedWrap : 1;
@@ -464,13 +465,13 @@
   bool hasAllowReassociation() const { return AllowReassociation; }
   bool hasNoFPExcept() const { return NoFPExcept; }
 
-  /// Clear any flags in this flag set that aren't also set in Flags.
-  /// If the given Flags are undefined then don't do anything.
+  /// Clear any flags in this flag set that aren't also set in Flags.  If the
+  /// given Flags are undefined then don't do anything with the FP flags.
   void intersectWith(const SDNodeFlags Flags) {
-    if (!Flags.isDefined())
-      return;
     NoUnsignedWrap &= Flags.NoUnsignedWrap;
     NoSignedWrap &= Flags.NoSignedWrap;
+    if (!Flags.isDefined())
+      return;
     Exact &= Flags.Exact;
     NoNaNs &= Flags.NoNaNs;
     NoInfs &= Flags.NoInfs;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D86871.288920.patch
Type: text/x-patch
Size: 2245 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200831/9ad90eec/attachment.bin>