[DAG] Early exit for flags in canCreateUndefOrPoison [nfc]

[preames]

This matches the style used in the Analysis version of this routine, and makes it less likely we'll miss a poison generating flag in future changes. Unlike IR, the check for poison generating flags doesn't need to switch over opcode since all nodes have the SDFlags storage.

[llvmbot]

@llvm/pr-subscribers-llvm-selectiondag

Author: Philip Reames (preames)

Changes

This matches the style used in the Analysis version of this routine, and makes it less likely we'll miss a poison generating flag in future changes. Unlike IR, the check for poison generating flags is near trivial since all nodes have the SDFlags storage, and all bits in that storage is poison generating.

---

Full diff: https://github.com/llvm/llvm-project/pull/89834.diff

2 Files Affected:

• (modified) llvm/include/llvm/CodeGen/SelectionDAGNodes.h (+7)
• (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+9-19)

diff --git a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
index 261f7e49e5c8ca..bf73015ed888ed 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
@@ -452,6 +452,13 @@ struct SDNodeFlags {
   bool hasNoFPExcept() const { return NoFPExcept; }
   bool hasUnpredictable() const { return Unpredictable; }
 
+  bool hasAny() const {
+    return NoUnsignedWrap || NoSignedWrap || Exact || Disjoint || NonNeg ||
+           NoNaNs || NoInfs || NoSignedZeros || AllowReciprocal ||
+           AllowContract || ApproximateFuncs || AllowReassociation ||
+           NoFPExcept || Unpredictable;
+  }
+
   /// Clear any flags in this flag set that aren't also set in Flags. All
   /// flags will be cleared if Flags are undefined.
   void intersectWith(const SDNodeFlags Flags) {
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index b63b8b893fdbf1..de0c5c0305ad57 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -5095,6 +5095,10 @@ bool SelectionDAG::canCreateUndefOrPoison(SDValue Op, const APInt &DemandedElts,
   if (VT.isScalableVector())
     return true;
 
+  // Matches hasPoisonGeneratingFlags().
+  if (ConsiderFlags && Op->getFlags().hasAny())
+    return true;
+
   unsigned Opcode = Op.getOpcode();
   switch (Opcode) {
   case ISD::FREEZE:
@@ -5134,34 +5138,20 @@ bool SelectionDAG::canCreateUndefOrPoison(SDValue Op, const APInt &DemandedElts,
       return true;
 
     const TargetOptions &Options = getTarget().Options;
-    return Options.NoNaNsFPMath || Options.NoInfsFPMath ||
-           (ConsiderFlags &&
-            (Op->getFlags().hasNoNaNs() || Op->getFlags().hasNoInfs()));
+    return Options.NoNaNsFPMath || Options.NoInfsFPMath;
   }
 
-  // Matches hasPoisonGeneratingFlags().
+  case ISD::OR:
   case ISD::ZERO_EXTEND:
-    return ConsiderFlags && Op->getFlags().hasNonNeg();
-
   case ISD::ADD:
   case ISD::SUB:
   case ISD::MUL:
-    // Matches hasPoisonGeneratingFlags().
-    return ConsiderFlags && (Op->getFlags().hasNoSignedWrap() ||
-                             Op->getFlags().hasNoUnsignedWrap());
+    // No poison except from flags (which is handled above)
+    return false;
 
   case ISD::SHL:
     // If the max shift amount isn't in range, then the shift can create poison.
-    if (!getValidMaximumShiftAmountConstant(Op, DemandedElts))
-      return true;
-
-    // Matches hasPoisonGeneratingFlags().
-    return ConsiderFlags && (Op->getFlags().hasNoSignedWrap() ||
-                             Op->getFlags().hasNoUnsignedWrap());
-
-  // Matches hasPoisonGeneratingFlags().
-  case ISD::OR:
-    return ConsiderFlags && Op->getFlags().hasDisjoint();
+    return !getValidMaximumShiftAmountConstant(Op, DemandedElts);
 
   case ISD::SCALAR_TO_VECTOR:
     // Check if we demand any upper (undef) elements.

[topperc]

I thought of the FMFs, only NoNaNs and NoInfs are poison generating.

[nikic]

That's my understanding as well.

[preames]

Dropped the ones which obviously correspond to IR level flags. Any clue on NoFPExcept or Unpredictable? As a guess, probably not poison generating?

[nikic]

Unpredictable is definitely not poison-generating.

NoFPExcept I'm not sure about, but I'd assume that one results in UB rather than poison.

[preames]

Closest I could find to a definition on NoFPExcept is from the constrained intrinsics (https://llvm.org/docs/LangRef.html#id2340), and I agree that appears non-poison generating. (Except maybe on FP flags? But I don't think we model it that way.) At the very least, the ValueTracking code doesn't seem to check for this property on constrained intrinsics.

[bjope]

How do we deal with (or want to deal with) things like

define i32 @foo(i32 %a) {
  %y = and i32 %a, 3
  %x = shl nuw nsw i32 1, %y
  ret i32 %x
}

The SHL DAG node will have the poison generating flags as well. But given that the shift amount is small this shift will never result in poison. Those flags are just "caching the information" that the shift won't sign wrap as detected by earlier value tracking rather than being put there to signal that the result might be poison.
Isn't it a bit weird that isGuaranteedNotToBeUndefOrPoison would return false for the SHL already without this patch?
By adding the early exit in canCreateUndefOrPoison, then it will be hard to make a more details analysis later in the function.

Maybe it is a bad thing that we both derive flags as nsw/nuw based on value tracking, while using the same flags to indicate properties of the input language (such as UB on signed overflow in C).

[preames]

@bjope I believe in your example the flags could be trivially re-inferred after any transform which drops them. (Well, or at least the major transform this analysis drives which is freeze hoisting/elimination.) As such, I'm not sure how useful strengthening the analysis would be.

However, at a macro level, I think you're raising a question which is well beyond the scope of this review. If in the future, we decide to handle your case differently, we would of course change the code. Going with a particular style now does not prevent us choosing a different style at a later point if the problem being solved has changed.

[nikic]

LGTM