[SelectionDAG] Add initial support for nneg flag on ISD::ZERO_EXTEND. #70872

topperc · 2023-10-31T23:17:38Z

This adds the nneg flag to SDNodeFlags and the node printing code. SelectionDAGBuilder will add this flag to the node if the target doesn't prefer sign extend.

A future RISC-V patch can remove the sign extend preference from SelectionDAGBuilder.

I've also added the flag to the DAG combine that converts ISD::SIGN_EXTEND to ISD::ZERO_EXTEND.

This adds the nneg flag to SDNodeFlags and the node printing code. SelectionDAGBuilder will add this flag to the node if the target doesn't prefer sign extend. A future RISC-V patch can remove the sign extend preference from SelectionDAGBuilder. I've also added the flag to the DAG combine that converts ISD::SIGN_EXTEND to ISD::ZERO_EXTEND.

llvmbot · 2023-10-31T23:18:49Z

@llvm/pr-subscribers-llvm-selectiondag

Author: Craig Topper (topperc)

Changes

This adds the nneg flag to SDNodeFlags and the node printing code. SelectionDAGBuilder will add this flag to the node if the target doesn't prefer sign extend.

A future RISC-V patch can remove the sign extend preference from SelectionDAGBuilder.

I've also added the flag to the DAG combine that converts ISD::SIGN_EXTEND to ISD::ZERO_EXTEND.

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

4 Files Affected:

(modified) llvm/include/llvm/CodeGen/SelectionDAGNodes.h (+7-3)
(modified) llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (+5-2)
(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+9-7)
(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp (+3)

diff --git a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
index 59c6feec8bcbfed..4df56aac4aa17ba 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
@@ -381,6 +381,7 @@ struct SDNodeFlags {
   bool NoUnsignedWrap : 1;
   bool NoSignedWrap : 1;
   bool Exact : 1;
+  bool NonNeg : 1;
   bool NoNaNs : 1;
   bool NoInfs : 1;
   bool NoSignedZeros : 1;
@@ -401,9 +402,9 @@ struct SDNodeFlags {
 public:
   /// Default constructor turns off all optimization flags.
   SDNodeFlags()
-      : NoUnsignedWrap(false), NoSignedWrap(false), Exact(false), NoNaNs(false),
-        NoInfs(false), NoSignedZeros(false), AllowReciprocal(false),
-        AllowContract(false), ApproximateFuncs(false),
+      : NoUnsignedWrap(false), NoSignedWrap(false), Exact(false), NonNeg(false),
+        NoNaNs(false), NoInfs(false), NoSignedZeros(false),
+        AllowReciprocal(false), AllowContract(false), ApproximateFuncs(false),
         AllowReassociation(false), NoFPExcept(false), Unpredictable(false) {}
 
   /// Propagate the fast-math-flags from an IR FPMathOperator.
@@ -421,6 +422,7 @@ struct SDNodeFlags {
   void setNoUnsignedWrap(bool b) { NoUnsignedWrap = b; }
   void setNoSignedWrap(bool b) { NoSignedWrap = b; }
   void setExact(bool b) { Exact = 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; }
@@ -435,6 +437,7 @@ struct SDNodeFlags {
   bool hasNoUnsignedWrap() const { return NoUnsignedWrap; }
   bool hasNoSignedWrap() const { return NoSignedWrap; }
   bool hasExact() const { return Exact; }
+  bool hasNonNeg() const { return NonNeg; }
   bool hasNoNaNs() const { return NoNaNs; }
   bool hasNoInfs() const { return NoInfs; }
   bool hasNoSignedZeros() const { return NoSignedZeros; }
@@ -451,6 +454,7 @@ struct SDNodeFlags {
     NoUnsignedWrap &= Flags.NoUnsignedWrap;
     NoSignedWrap &= Flags.NoSignedWrap;
     Exact &= Flags.Exact;
+    NonNeg &= Flags.Exact;
     NoNaNs &= Flags.NoNaNs;
     NoInfs &= Flags.NoInfs;
     NoSignedZeros &= Flags.NoSignedZeros;
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index ca5bd4952866886..8c1282274372088 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -13484,8 +13484,11 @@ 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))
-    return DAG.getNode(ISD::ZERO_EXTEND, DL, VT, N0);
+      DAG.SignBitIsZero(N0)) {
+    SDNodeFlags Flags;
+    Flags.setNonNeg(true);
+    return DAG.getNode(ISD::ZERO_EXTEND, DL, VT, N0, Flags);
+  }
 
   if (SDValue NewVSel = matchVSelectOpSizesWithSetCC(N))
     return NewVSel;
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 229f220d8460bda..e4832216850efd9 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -3527,18 +3527,20 @@ void SelectionDAGBuilder::visitZExt(const User &I) {
   auto &TLI = DAG.getTargetLoweringInfo();
   EVT DestVT = TLI.getValueType(DAG.getDataLayout(), I.getType());
 
-  // Since we don't yet have a representation of zext nneg in SDAG or MI,
-  // eagerly use the information to canonicalize towards sign_extend if
-  // that is the target's preference.  TODO: Add nneg support to the
-  // SDAG and MI representations.
-  if (auto *PNI = dyn_cast<PossiblyNonNegInst>(&I);
-      PNI && PNI->hasNonNeg() &&
+  SDNodeFlags Flags;
+  if (auto *PNI = dyn_cast<PossiblyNonNegInst>(&I))
+    Flags.setNonNeg(PNI->hasNonNeg());
+
+  // Eagerly use nonneg information to canonicalize towards sign_extend if
+  // that is the target's preference.
+  // TODO: Let the target do this later.
+  if (Flags.hasNonNeg() &&
       TLI.isSExtCheaperThanZExt(N.getValueType(), DestVT)) {
     setValue(&I, DAG.getNode(ISD::SIGN_EXTEND, getCurSDLoc(), DestVT, N));
     return;
   }
 
-  setValue(&I, DAG.getNode(ISD::ZERO_EXTEND, getCurSDLoc(), DestVT, N));
+  setValue(&I, DAG.getNode(ISD::ZERO_EXTEND, getCurSDLoc(), DestVT, N, Flags));
 }
 
 void SelectionDAGBuilder::visitSExt(const User &I) {
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
index a92111ca23656eb..78cc60084068a5f 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
@@ -597,6 +597,9 @@ void SDNode::print_details(raw_ostream &OS, const SelectionDAG *G) const {
   if (getFlags().hasExact())
     OS << " exact";
 
+  if (getFlags().hasNonNeg())
+    OS << " nneg";
+
   if (getFlags().hasNoNaNs())
     OS << " nnan";

nikic

You need to update the SDAG canCreateUndefOrPoison() implementation to account for the new flag.

This generally looks ok to me, but I'll leave it to people who do more backend work to decide whether having this flag in SDAG is worthwhile. Given how target-specific SDAG is, I could see the argument for just using SEXT based on target preference and not adding the complexity of additional poison flags.

preames · 2023-11-01T14:56:30Z

Do we need to add the flag to the zext_in_reg nodes? This question is complicated by the fact that comments in DAGCombine mention such a node, but it doesn't appear to be in the ISDOpcodes.h list.

Can you update the comment for ZERO_EXTEND in ISDOpcodes.h to mention the flag and the implied poison semantics?

For context, we're definitely going to need this for RISCV. At the moment, I set us up to prefer SIGN_EXTEND based on the target preference, and I see some cases in benchmarks where this hurt codegen because we missed a zext fold or zeroing bits is easier in a particular case (say, and with smallish constant).

topperc · 2023-11-01T16:38:34Z

Do we need to add the flag to the zext_in_reg nodes? This question is complicated by the fact that comments in DAGCombine mention such a node, but it doesn't appear to be in the ISDOpcodes.h list.

zext_in_reg is ISD::AND with a trailing ones mask.

preames

LGTM

nikic

Should also update SimplifyDemanded to drop that flag.

nikic · 2023-11-01T17:35:18Z

llvm/include/llvm/CodeGen/ISDOpcodes.h

-  /// ZERO_EXTEND - Used for integer types, zeroing the new bits.
+  /// ZERO_EXTEND - Used for integer types, zeroing the new bits. Can carry
+  /// the NonNeg SDNodeFlag to indicate that the input is known to be
+  /// non-negative. If the flag is present and the input is negative, the reuslt


Suggested change

/// non-negative. If the flag is present and the input is negative, the reuslt

/// non-negative. If the flag is present and the input is negative, the result

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

nikic · 2023-11-01T17:39:16Z

Do we need to add the flag to the zext_in_reg nodes? This question is complicated by the fact that comments in DAGCombine mention such a node, but it doesn't appear to be in the ISDOpcodes.h list.

zext_in_reg is ISD::AND with a trailing ones mask.

There is ZERO_EXTEND_VECTOR_INREG though, so I guess the question applies to that one.

topperc requested review from nikic, preames and efriedma-quic October 31, 2023 23:17

llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Oct 31, 2023

nikic reviewed Nov 1, 2023

View reviewed changes

topperc added 2 commits November 1, 2023 09:41

Update canCreateUndefOrPoison

0919bde

Document poison behavior in ISDOpcodes.h

1c98baa

preames approved these changes Nov 1, 2023

View reviewed changes

nikic reviewed Nov 1, 2023

View reviewed changes

topperc added 3 commits November 1, 2023 10:47

Fix typo

4107900

Drop flag in SimplifyDemandedBits.

8463788

Fix typo

c3e33bd

topperc merged commit 70b35ec into llvm:main Nov 3, 2023

topperc deleted the pr/sdnode-nonneg branch November 18, 2023 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SelectionDAG] Add initial support for nneg flag on ISD::ZERO_EXTEND. #70872

[SelectionDAG] Add initial support for nneg flag on ISD::ZERO_EXTEND. #70872

Uh oh!

topperc commented Oct 31, 2023

Uh oh!

llvmbot commented Oct 31, 2023

Uh oh!

nikic left a comment

Uh oh!

preames commented Nov 1, 2023

Uh oh!

topperc commented Nov 1, 2023

Uh oh!

preames left a comment

Uh oh!

nikic left a comment

Uh oh!

nikic Nov 1, 2023

Uh oh!

Uh oh!

nikic commented Nov 1, 2023

Uh oh!

Uh oh!

	/// non-negative. If the flag is present and the input is negative, the reuslt
	/// non-negative. If the flag is present and the input is negative, the result

[SelectionDAG] Add initial support for nneg flag on ISD::ZERO_EXTEND. #70872

[SelectionDAG] Add initial support for nneg flag on ISD::ZERO_EXTEND. #70872

Uh oh!

Conversation

topperc commented Oct 31, 2023

Uh oh!

llvmbot commented Oct 31, 2023

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

preames commented Nov 1, 2023

Uh oh!

topperc commented Nov 1, 2023

Uh oh!

preames left a comment

Choose a reason for hiding this comment

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

nikic Nov 1, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikic commented Nov 1, 2023

Uh oh!

Uh oh!