Skip to content

[SDAG] Simplify SDNodeFlags with bitwise logic #114061

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 31, 2024
Merged

Conversation

dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Oct 29, 2024

This patch allows using enumeration values directly and simplifies the implementation with bitwise logic. It addresses the comment in #113808 (comment).

@llvmbot
Copy link
Member

llvmbot commented Oct 29, 2024

@llvm/pr-subscribers-llvm-selectiondag
@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-backend-aarch64

Author: Yingwei Zheng (dtcxzyw)

Changes

This patch converts SDNodeFlags into an enumeration as we did for FastMathFlags. It simplifies the implementation and improves compile-time.

The first commit only modifies SDNodeFlags without changing its API.
The second commit allows using enumeration values directly and simplifies the implementation with bitwise logic. It addresses the comment in #113808 (comment).

Compile-time improvement: http://llvm-compile-time-tracker.com/compare.php?from=8e6856e27859c90c5337a8328848b0959fe409fe&to=b7de8427ff84035b7e5c9513d004d486918be605&stat=instructions%3Au


Patch is 38.79 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/114061.diff

16 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/SDPatternMatch.h (+4-12)
  • (modified) llvm/include/llvm/CodeGen/SelectionDAG.h (+2-6)
  • (modified) llvm/include/llvm/CodeGen/SelectionDAGNodes.h (+80-89)
  • (modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (+15-25)
  • (modified) llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp (+2-5)
  • (modified) llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp (+3-3)
  • (modified) llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp (+3-9)
  • (modified) llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp (+1-3)
  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+1-1)
  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+7-10)
  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp (+2-5)
  • (modified) llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp (+21-56)
  • (modified) llvm/lib/Target/AArch64/AArch64ISelLowering.cpp (+4-11)
  • (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+6-11)
  • (modified) llvm/lib/Target/SystemZ/SystemZISelLowering.cpp (+1-4)
  • (modified) llvm/unittests/CodeGen/SelectionDAGPatternMatchTest.cpp (+3-6)
diff --git a/llvm/include/llvm/CodeGen/SDPatternMatch.h b/llvm/include/llvm/CodeGen/SDPatternMatch.h
index b3e249b7ebd5c4..96667952a16efc 100644
--- a/llvm/include/llvm/CodeGen/SDPatternMatch.h
+++ b/llvm/include/llvm/CodeGen/SDPatternMatch.h
@@ -533,9 +533,7 @@ struct BinaryOpc_match {
       if (!Flags.has_value())
         return true;
 
-      SDNodeFlags TmpFlags = *Flags;
-      TmpFlags.intersectWith(N->getFlags());
-      return TmpFlags == *Flags;
+      return (*Flags & N->getFlags()) == *Flags;
     }
 
     return false;
@@ -668,9 +666,7 @@ inline BinaryOpc_match<LHS, RHS, true> m_Or(const LHS &L, const RHS &R) {
 template <typename LHS, typename RHS>
 inline BinaryOpc_match<LHS, RHS, true> m_DisjointOr(const LHS &L,
                                                     const RHS &R) {
-  SDNodeFlags Flags;
-  Flags.setDisjoint(true);
-  return BinaryOpc_match<LHS, RHS, true>(ISD::OR, L, R, Flags);
+  return BinaryOpc_match<LHS, RHS, true>(ISD::OR, L, R, SDNodeFlags::Disjoint);
 }
 
 template <typename LHS, typename RHS>
@@ -813,9 +809,7 @@ template <typename Opnd_P, bool ExcludeChain = false> struct UnaryOpc_match {
       if (!Flags.has_value())
         return true;
 
-      SDNodeFlags TmpFlags = *Flags;
-      TmpFlags.intersectWith(N->getFlags());
-      return TmpFlags == *Flags;
+      return (*Flags & N->getFlags()) == *Flags;
     }
 
     return false;
@@ -848,9 +842,7 @@ template <typename Opnd> inline UnaryOpc_match<Opnd> m_ZExt(const Opnd &Op) {
 
 template <typename Opnd>
 inline UnaryOpc_match<Opnd> m_NNegZExt(const Opnd &Op) {
-  SDNodeFlags Flags;
-  Flags.setNonNeg(true);
-  return UnaryOpc_match<Opnd>(ISD::ZERO_EXTEND, Op, Flags);
+  return UnaryOpc_match<Opnd>(ISD::ZERO_EXTEND, Op, SDNodeFlags::NonNeg);
 }
 
 template <typename Opnd> inline auto m_SExt(const Opnd &Op) {
diff --git a/llvm/include/llvm/CodeGen/SelectionDAG.h b/llvm/include/llvm/CodeGen/SelectionDAG.h
index e82bdb6906163c..db111b0875a6ee 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAG.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAG.h
@@ -1064,17 +1064,13 @@ class SelectionDAG {
   /// addressing some offset of an object. i.e. if a load is split into multiple
   /// components, create an add nuw from the base pointer to the offset.
   SDValue getObjectPtrOffset(const SDLoc &SL, SDValue Ptr, TypeSize Offset) {
-    SDNodeFlags Flags;
-    Flags.setNoUnsignedWrap(true);
-    return getMemBasePlusOffset(Ptr, Offset, SL, Flags);
+    return getMemBasePlusOffset(Ptr, Offset, SL, SDNodeFlags::NoUnsignedWrap);
   }
 
   SDValue getObjectPtrOffset(const SDLoc &SL, SDValue Ptr, SDValue Offset) {
     // The object itself can't wrap around the address space, so it shouldn't be
     // possible for the adds of the offsets to the split parts to overflow.
-    SDNodeFlags Flags;
-    Flags.setNoUnsignedWrap(true);
-    return getMemBasePlusOffset(Ptr, Offset, SL, Flags);
+    return getMemBasePlusOffset(Ptr, Offset, SL, SDNodeFlags::NoUnsignedWrap);
   }
 
   /// Return a new CALLSEQ_START node, that starts new call frame, in which
diff --git a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
index bda0120a2df4aa..91850c699ecbff 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
@@ -378,36 +378,46 @@ template<> struct simplify_type<SDUse> {
 /// the backend.
 struct SDNodeFlags {
 private:
-  bool NoUnsignedWrap : 1;
-  bool NoSignedWrap : 1;
-  bool Exact : 1;
-  bool Disjoint : 1;
-  bool NonNeg : 1;
-  bool NoNaNs : 1;
-  bool NoInfs : 1;
-  bool NoSignedZeros : 1;
-  bool AllowReciprocal : 1;
-  bool AllowContract : 1;
-  bool ApproximateFuncs : 1;
-  bool AllowReassociation : 1;
-
-  // We assume instructions do not raise floating-point exceptions by default,
-  // and only those marked explicitly may do so.  We could choose to represent
-  // this via a positive "FPExcept" flags like on the MI level, but having a
-  // negative "NoFPExcept" flag here makes the flag intersection logic more
-  // straightforward.
-  bool NoFPExcept : 1;
-  // Instructions with attached 'unpredictable' metadata on IR level.
-  bool Unpredictable : 1;
+  friend class SDNode;
+
+  unsigned Flags = 0;
+
+  template <unsigned Flag> void setFlag(bool B) {
+    Flags = (Flags & ~Flag) | (B ? Flag : 0);
+  }
 
 public:
+  enum {
+    None = 0,
+    NoUnsignedWrap = 1 << 0,
+    NoSignedWrap = 1 << 1,
+    NoWrap = NoUnsignedWrap | NoSignedWrap,
+    Exact = 1 << 2,
+    Disjoint = 1 << 3,
+    NonNeg = 1 << 4,
+    NoNaNs = 1 << 5,
+    NoInfs = 1 << 6,
+    NoSignedZeros = 1 << 7,
+    AllowReciprocal = 1 << 8,
+    AllowContract = 1 << 9,
+    ApproximateFuncs = 1 << 10,
+    AllowReassociation = 1 << 11,
+
+    // We assume instructions do not raise floating-point exceptions by default,
+    // and only those marked explicitly may do so.  We could choose to represent
+    // this via a positive "FPExcept" flags like on the MI level, but having a
+    // negative "NoFPExcept" flag here makes the flag intersection logic more
+    // straightforward.
+    NoFPExcept = 1 << 12,
+    // Instructions with attached 'unpredictable' metadata on IR level.
+    Unpredictable = 1 << 13,
+
+    PoisonGeneratingFlags = NoUnsignedWrap | NoSignedWrap | Exact | Disjoint |
+                            NonNeg | NoNaNs | NoInfs,
+  };
+
   /// Default constructor turns off all optimization flags.
-  SDNodeFlags()
-      : NoUnsignedWrap(false), NoSignedWrap(false), Exact(false),
-        Disjoint(false), NonNeg(false), NoNaNs(false), NoInfs(false),
-        NoSignedZeros(false), AllowReciprocal(false), AllowContract(false),
-        ApproximateFuncs(false), AllowReassociation(false), NoFPExcept(false),
-        Unpredictable(false) {}
+  SDNodeFlags(unsigned Flags = SDNodeFlags::None) : Flags(Flags) {}
 
   /// Propagate the fast-math-flags from an IR FPMathOperator.
   void copyFMF(const FPMathOperator &FPMO) {
@@ -421,71 +431,54 @@ struct SDNodeFlags {
   }
 
   // These are mutators for each flag.
-  void setNoUnsignedWrap(bool b) { NoUnsignedWrap = b; }
-  void setNoSignedWrap(bool b) { NoSignedWrap = b; }
-  void setExact(bool b) { Exact = b; }
-  void setDisjoint(bool b) { Disjoint = b; }
-  void setNonNeg(bool b) { NonNeg = b; }
-  void setNoNaNs(bool b) { NoNaNs = b; }
-  void setNoInfs(bool b) { NoInfs = b; }
-  void setNoSignedZeros(bool b) { NoSignedZeros = b; }
-  void setAllowReciprocal(bool b) { AllowReciprocal = b; }
-  void setAllowContract(bool b) { AllowContract = b; }
-  void setApproximateFuncs(bool b) { ApproximateFuncs = b; }
-  void setAllowReassociation(bool b) { AllowReassociation = b; }
-  void setNoFPExcept(bool b) { NoFPExcept = b; }
-  void setUnpredictable(bool b) { Unpredictable = b; }
+  void setNoUnsignedWrap(bool b) { setFlag<NoUnsignedWrap>(b); }
+  void setNoSignedWrap(bool b) { setFlag<NoSignedWrap>(b); }
+  void setExact(bool b) { setFlag<Exact>(b); }
+  void setDisjoint(bool b) { setFlag<Disjoint>(b); }
+  void setNonNeg(bool b) { setFlag<NonNeg>(b); }
+  void setNoNaNs(bool b) { setFlag<NoNaNs>(b); }
+  void setNoInfs(bool b) { setFlag<NoInfs>(b); }
+  void setNoSignedZeros(bool b) { setFlag<NoSignedZeros>(b); }
+  void setAllowReciprocal(bool b) { setFlag<AllowReciprocal>(b); }
+  void setAllowContract(bool b) { setFlag<AllowContract>(b); }
+  void setApproximateFuncs(bool b) { setFlag<ApproximateFuncs>(b); }
+  void setAllowReassociation(bool b) { setFlag<AllowReassociation>(b); }
+  void setNoFPExcept(bool b) { setFlag<NoFPExcept>(b); }
+  void setUnpredictable(bool b) { setFlag<Unpredictable>(b); }
 
   // These are accessors for each flag.
-  bool hasNoUnsignedWrap() const { return NoUnsignedWrap; }
-  bool hasNoSignedWrap() const { return NoSignedWrap; }
-  bool hasExact() const { return Exact; }
-  bool hasDisjoint() const { return Disjoint; }
-  bool hasNonNeg() const { return NonNeg; }
-  bool hasNoNaNs() const { return NoNaNs; }
-  bool hasNoInfs() const { return NoInfs; }
-  bool hasNoSignedZeros() const { return NoSignedZeros; }
-  bool hasAllowReciprocal() const { return AllowReciprocal; }
-  bool hasAllowContract() const { return AllowContract; }
-  bool hasApproximateFuncs() const { return ApproximateFuncs; }
-  bool hasAllowReassociation() const { return AllowReassociation; }
-  bool hasNoFPExcept() const { return NoFPExcept; }
-  bool hasUnpredictable() const { return Unpredictable; }
+  bool hasNoUnsignedWrap() const { return Flags & NoUnsignedWrap; }
+  bool hasNoSignedWrap() const { return Flags & NoSignedWrap; }
+  bool hasExact() const { return Flags & Exact; }
+  bool hasDisjoint() const { return Flags & Disjoint; }
+  bool hasNonNeg() const { return Flags & NonNeg; }
+  bool hasNoNaNs() const { return Flags & NoNaNs; }
+  bool hasNoInfs() const { return Flags & NoInfs; }
+  bool hasNoSignedZeros() const { return Flags & NoSignedZeros; }
+  bool hasAllowReciprocal() const { return Flags & AllowReciprocal; }
+  bool hasAllowContract() const { return Flags & AllowContract; }
+  bool hasApproximateFuncs() const { return Flags & ApproximateFuncs; }
+  bool hasAllowReassociation() const { return Flags & AllowReassociation; }
+  bool hasNoFPExcept() const { return Flags & NoFPExcept; }
+  bool hasUnpredictable() const { return Flags & Unpredictable; }
 
   bool operator==(const SDNodeFlags &Other) const {
-    return NoUnsignedWrap == Other.NoUnsignedWrap &&
-           NoSignedWrap == Other.NoSignedWrap && Exact == Other.Exact &&
-           Disjoint == Other.Disjoint && NonNeg == Other.NonNeg &&
-           NoNaNs == Other.NoNaNs && NoInfs == Other.NoInfs &&
-           NoSignedZeros == Other.NoSignedZeros &&
-           AllowReciprocal == Other.AllowReciprocal &&
-           AllowContract == Other.AllowContract &&
-           ApproximateFuncs == Other.ApproximateFuncs &&
-           AllowReassociation == Other.AllowReassociation &&
-           NoFPExcept == Other.NoFPExcept &&
-           Unpredictable == Other.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) {
-    NoUnsignedWrap &= Flags.NoUnsignedWrap;
-    NoSignedWrap &= Flags.NoSignedWrap;
-    Exact &= Flags.Exact;
-    Disjoint &= Flags.Disjoint;
-    NonNeg &= Flags.NonNeg;
-    NoNaNs &= Flags.NoNaNs;
-    NoInfs &= Flags.NoInfs;
-    NoSignedZeros &= Flags.NoSignedZeros;
-    AllowReciprocal &= Flags.AllowReciprocal;
-    AllowContract &= Flags.AllowContract;
-    ApproximateFuncs &= Flags.ApproximateFuncs;
-    AllowReassociation &= Flags.AllowReassociation;
-    NoFPExcept &= Flags.NoFPExcept;
-    Unpredictable &= Flags.Unpredictable;
+    return Flags == Other.Flags;
   }
+  void operator&=(const SDNodeFlags &OtherFlags) { Flags &= OtherFlags.Flags; }
+  void operator|=(const SDNodeFlags &OtherFlags) { Flags |= OtherFlags.Flags; }
 };
 
+inline SDNodeFlags operator|(SDNodeFlags LHS, SDNodeFlags RHS) {
+  LHS |= RHS;
+  return LHS;
+}
+
+inline SDNodeFlags operator&(SDNodeFlags LHS, SDNodeFlags RHS) {
+  LHS &= RHS;
+  return LHS;
+}
+
 /// Represents one node in the SelectionDAG.
 ///
 class SDNode : public FoldingSetNode, public ilist_node<SDNode> {
@@ -1023,16 +1016,14 @@ END_TWO_BYTE_PACK()
 
   SDNodeFlags getFlags() const { return Flags; }
   void setFlags(SDNodeFlags NewFlags) { Flags = NewFlags; }
+  void clearFlags(unsigned Mask) { Flags &= ~Mask; }
 
   /// Clear any flags in this node that aren't also set in Flags.
   /// If Flags is not in a defined state then this has no effect.
   void intersectFlagsWith(const SDNodeFlags Flags);
 
   bool hasPoisonGeneratingFlags() const {
-    SDNodeFlags Flags = getFlags();
-    return Flags.hasNoUnsignedWrap() || Flags.hasNoSignedWrap() ||
-           Flags.hasExact() || Flags.hasDisjoint() || Flags.hasNonNeg() ||
-           Flags.hasNoNaNs() || Flags.hasNoInfs();
+    return Flags.Flags & SDNodeFlags::PoisonGeneratingFlags;
   }
 
   void setCFIType(uint32_t Type) { CFIType = Type; }
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index b800204d917503..c5d8af102123db 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -1210,7 +1210,7 @@ SDValue DAGCombiner::reassociateOpsCommutative(unsigned Opc, const SDLoc &DL,
     SDNodeFlags NewFlags;
     if (N0.getOpcode() == ISD::ADD && N0->getFlags().hasNoUnsignedWrap() &&
         Flags.hasNoUnsignedWrap())
-      NewFlags.setNoUnsignedWrap(true);
+      NewFlags = SDNodeFlags::NoUnsignedWrap;
 
     if (DAG.isConstantIntBuildVectorOrConstantInt(N1)) {
       // Reassociate: (op (op x, c1), c2) -> (op x, (op c1, c2))
@@ -2892,11 +2892,11 @@ SDValue DAGCombiner::visitADDLike(SDNode *N) {
         if (N->getFlags().hasNoUnsignedWrap() &&
             N0->getFlags().hasNoUnsignedWrap() &&
             N0.getOperand(0)->getFlags().hasNoUnsignedWrap()) {
-          Flags.setNoUnsignedWrap(true);
+          Flags = SDNodeFlags::NoUnsignedWrap;
           if (N->getFlags().hasNoSignedWrap() &&
               N0->getFlags().hasNoSignedWrap() &&
               N0.getOperand(0)->getFlags().hasNoSignedWrap())
-            Flags.setNoSignedWrap(true);
+            Flags |= SDNodeFlags::NoSignedWrap;
         }
         SDValue Mul = DAG.getNode(ISD::MUL, SDLoc(N1), VT, A,
                                   DAG.getConstant(CM, DL, VT), Flags);
@@ -2920,12 +2920,12 @@ SDValue DAGCombiner::visitADDLike(SDNode *N) {
             N0->getFlags().hasNoUnsignedWrap() &&
             OMul->getFlags().hasNoUnsignedWrap() &&
             OMul.getOperand(0)->getFlags().hasNoUnsignedWrap()) {
-          Flags.setNoUnsignedWrap(true);
+          Flags = SDNodeFlags::NoUnsignedWrap;
           if (N->getFlags().hasNoSignedWrap() &&
               N0->getFlags().hasNoSignedWrap() &&
               OMul->getFlags().hasNoSignedWrap() &&
               OMul.getOperand(0)->getFlags().hasNoSignedWrap())
-            Flags.setNoSignedWrap(true);
+            Flags |= SDNodeFlags::NoSignedWrap;
         }
         SDValue Mul = DAG.getNode(ISD::MUL, SDLoc(N1), VT, A,
                                   DAG.getConstant(CM, DL, VT), Flags);
@@ -2987,11 +2987,8 @@ SDValue DAGCombiner::visitADD(SDNode *N) {
 
   // fold (a+b) -> (a|b) iff a and b share no bits.
   if ((!LegalOperations || TLI.isOperationLegal(ISD::OR, VT)) &&
-      DAG.haveNoCommonBitsSet(N0, N1)) {
-    SDNodeFlags Flags;
-    Flags.setDisjoint(true);
-    return DAG.getNode(ISD::OR, DL, VT, N0, N1, Flags);
-  }
+      DAG.haveNoCommonBitsSet(N0, N1))
+    return DAG.getNode(ISD::OR, DL, VT, N0, N1, SDNodeFlags::Disjoint);
 
   // Fold (add (vscale * C0), (vscale * C1)) to (vscale * (C0 + C1)).
   if (N0.getOpcode() == ISD::VSCALE && N1.getOpcode() == ISD::VSCALE) {
@@ -9547,11 +9544,8 @@ SDValue DAGCombiner::visitXOR(SDNode *N) {
 
   // fold (a^b) -> (a|b) iff a and b share no bits.
   if ((!LegalOperations || TLI.isOperationLegal(ISD::OR, VT)) &&
-      DAG.haveNoCommonBitsSet(N0, N1)) {
-    SDNodeFlags Flags;
-    Flags.setDisjoint(true);
-    return DAG.getNode(ISD::OR, DL, VT, N0, N1, Flags);
-  }
+      DAG.haveNoCommonBitsSet(N0, N1))
+    return DAG.getNode(ISD::OR, DL, VT, N0, N1, SDNodeFlags::Disjoint);
 
   // look for 'add-like' folds:
   // XOR(N0,MIN_SIGNED_VALUE) == ADD(N0,MIN_SIGNED_VALUE)
@@ -10201,7 +10195,7 @@ SDValue DAGCombiner::visitSHL(SDNode *N) {
       SDNodeFlags Flags;
       // Preserve the disjoint flag for Or.
       if (N0.getOpcode() == ISD::OR && N0->getFlags().hasDisjoint())
-        Flags.setDisjoint(true);
+        Flags = SDNodeFlags::Disjoint;
       return DAG.getNode(N0.getOpcode(), DL, VT, Shl0, Shl1, Flags);
     }
   }
@@ -13913,11 +13907,8 @@ SDValue DAGCombiner::visitSIGN_EXTEND(SDNode *N) {
   // fold (sext x) -> (zext x) if the sign bit is known zero.
   if (!TLI.isSExtCheaperThanZExt(N0.getValueType(), VT) &&
       (!LegalOperations || TLI.isOperationLegal(ISD::ZERO_EXTEND, VT)) &&
-      DAG.SignBitIsZero(N0)) {
-    SDNodeFlags Flags;
-    Flags.setNonNeg(true);
-    return DAG.getNode(ISD::ZERO_EXTEND, DL, VT, N0, Flags);
-  }
+      DAG.SignBitIsZero(N0))
+    return DAG.getNode(ISD::ZERO_EXTEND, DL, VT, N0, SDNodeFlags::NonNeg);
 
   if (SDValue NewVSel = matchVSelectOpSizesWithSetCC(N))
     return NewVSel;
@@ -14798,10 +14789,9 @@ SDValue DAGCombiner::reduceLoadWidth(SDNode *N) {
   uint64_t PtrOff = PtrAdjustmentInBits / 8;
   SDLoc DL(LN0);
   // The original load itself didn't wrap, so an offset within it doesn't.
-  SDNodeFlags Flags;
-  Flags.setNoUnsignedWrap(true);
-  SDValue NewPtr = DAG.getMemBasePlusOffset(
-      LN0->getBasePtr(), TypeSize::getFixed(PtrOff), DL, Flags);
+  SDValue NewPtr =
+      DAG.getMemBasePlusOffset(LN0->getBasePtr(), TypeSize::getFixed(PtrOff),
+                               DL, SDNodeFlags::NoUnsignedWrap);
   AddToWorklist(NewPtr.getNode());
 
   SDValue Load;
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
index e0a03383358b76..a139c5251cded8 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
@@ -1697,12 +1697,9 @@ SDValue SelectionDAGLegalize::ExpandFCOPYSIGN(SDNode *Node) const {
     SignBit = DAG.getNode(ISD::TRUNCATE, DL, MagVT, SignBit);
   }
 
-  SDNodeFlags Flags;
-  Flags.setDisjoint(true);
-
   // Store the part with the modified sign and convert back to float.
-  SDValue CopiedSign =
-      DAG.getNode(ISD::OR, DL, MagVT, ClearedSign, SignBit, Flags);
+  SDValue CopiedSign = DAG.getNode(ISD::OR, DL, MagVT, ClearedSign, SignBit,
+                                   SDNodeFlags::Disjoint);
 
   return modifySignAsInt(MagAsInt, DL, CopiedSign);
 }
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
index ee9c95c8593766..45487c887b74dd 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
@@ -4674,9 +4674,9 @@ void DAGTypeLegalizer::ExpandIntRes_ShiftThroughStack(SDNode *N, SDValue &Lo,
       DAG.getNode(ISD::SHL, dl, ShAmtVT, SrlTmp,
                   DAG.getConstant(Log2_32(ShiftUnitInBits), dl, ShAmtVT));
 
-  Flags.setExact(true);
-  SDValue ByteOffset = DAG.getNode(ISD::SRL, dl, ShAmtVT, BitOffset,
-                                   DAG.getConstant(3, dl, ShAmtVT), Flags);
+  SDValue ByteOffset =
+      DAG.getNode(ISD::SRL, dl, ShAmtVT, BitOffset,
+                  DAG.getConstant(3, dl, ShAmtVT), SDNodeFlags::Exact);
   // And clamp it, because OOB load is an immediate UB,
   // while shift overflow would have *just* been poison.
   ByteOffset = DAG.getNode(ISD::AND, dl, ShAmtVT, ByteOffset,
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
index a8042fc3e7a69a..89d61416e575b1 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
@@ -1699,11 +1699,8 @@ SDValue VectorLegalizer::ExpandVP_FCOPYSIGN(SDNode *Node) {
   SDValue ClearedSign =
       DAG.getNode(ISD::VP_AND, DL, IntVT, Mag, ClearSignMask, Mask, EVL);
 
-  SDNodeFlags Flags;
-  Flags.setDisjoint(true);
-
   SDValue CopiedSign = DAG.getNode(ISD::VP_OR, DL, IntVT, ClearedSign, SignBit,
-                                   Mask, EVL, Flags);
+                                   Mask, EVL, SDNodeFlags::Disjoint);
 
   return DAG.getNode(ISD::BITCAST, DL, VT, CopiedSign);
 }
@@ -1885,11 +1882,8 @@ SDValue VectorLegalizer::ExpandFCOPYSIGN(SDNode *Node) {
       APInt::getSignedMaxValue(IntVT.getScalarSizeInBits()), DL, IntVT);
   SDValue ClearedSign = DAG.getNode(ISD::AND, DL, IntVT, Mag, ClearSignMask);
 
-  SDNodeFlags Flags;
-  Flags.setDisjoint(true);
-
-  SDValue CopiedSign =
-      DAG.getNode(ISD::OR, DL, IntVT, ClearedSign, SignBit, Flags);
+  SDValue CopiedSign = DAG.getNode(ISD::OR, DL, IntVT, ClearedSign, SignBit,
+                                   SDNodeFlags::Disjoint);
 
   return DAG.getNode(ISD::BITCAST, DL, VT, CopiedSign);
 }
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
index 50e2a923699c8a..bdc4755b5e6ca0 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorType...
[truncated]

Copy link
Contributor

@antoniofrighetto antoniofrighetto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the whole change supposed to be part of a NFC, isn't it?

@dtcxzyw
Copy link
Member Author

dtcxzyw commented Oct 29, 2024

Is the whole change supposed to be part of a NFC, isn't it?

The second commit breaks SDNodeFlags API.

@goldsteinn
Copy link
Contributor

IMO since github doesn't have a great interface for reviewing single-commits in a series, you should split the first commit to its own proper NFC PR.

@dtcxzyw dtcxzyw changed the title [SDAG] Convert SDNodeFlags into an enumeration [SDAG] Simplifies SDNodeFlags with bitwise logic Oct 30, 2024
@@ -1006,6 +1016,7 @@ END_TWO_BYTE_PACK()

SDNodeFlags getFlags() const { return Flags; }
void setFlags(SDNodeFlags NewFlags) { Flags = NewFlags; }
void clearFlags(unsigned Mask) { Flags &= ~Mask; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interface here is a little confusing now. The existence of clearFlags could make someone think that setFlags would have OR semantics.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any suggestion on the naming? maskOutFlags? dropFlags?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with dropFlags

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that craig mentions it, I think the setFlags is still unclear given that the rest of the set* API does use |=. Maybe setFlags -> initFlags?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Emm... It is likely to break some downstream implementations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough;

@dtcxzyw dtcxzyw requested a review from preames October 30, 2024 06:50
@dtcxzyw dtcxzyw changed the title [SDAG] Simplifies SDNodeFlags with bitwise logic [SDAG] Simplify SDNodeFlags with bitwise logic Oct 30, 2024
Copy link
Contributor

@antoniofrighetto antoniofrighetto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@dtcxzyw dtcxzyw merged commit cf9d1c1 into llvm:main Oct 31, 2024
6 of 8 checks passed
@dtcxzyw dtcxzyw deleted the perf/sdflags branch October 31, 2024 00:10
smallp-o-p pushed a commit to smallp-o-p/llvm-project that referenced this pull request Nov 3, 2024
This patch allows using enumeration values directly and simplifies the
implementation with bitwise logic. It addresses the comment in
llvm#113808 (comment).
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
This patch allows using enumeration values directly and simplifies the
implementation with bitwise logic. It addresses the comment in
llvm#113808 (comment).
qiaojbao pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Dec 4, 2024
…1f64a35a2

Local branch amd-gfx f7d1f64 Merged main:85f3d5ca4994ff70a72f6ad81948bf4721e15ef1 into amd-gfx:7a21ca85ec98
Remote branch main cf9d1c1 [SDAG] Simplify `SDNodeFlags` with bitwise logic (llvm#114061)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants