Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@RKSimon, Am i on the right track? |
|
@llvm/pr-subscribers-llvm-selectiondag Author: Aryan Kadole (ak1932) Changeswip: Still need to find a fold in DAGCombiner and add more extensive unit testing. Resolves #174327 Full diff: https://github.com/llvm/llvm-project/pull/175191.diff 4 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/SDPatternMatch.h b/llvm/include/llvm/CodeGen/SDPatternMatch.h
index 026ee035fcf54..6cf3bb115448e 100644
--- a/llvm/include/llvm/CodeGen/SDPatternMatch.h
+++ b/llvm/include/llvm/CodeGen/SDPatternMatch.h
@@ -1173,6 +1173,46 @@ inline SpecificFP_match m_SpecificFP(double V) {
return SpecificFP_match(APFloat(V));
}
+struct Negative_match {
+ template <typename MatchContext>
+ bool match(const MatchContext &M, SDValue N) const {
+ const SelectionDAG *DAG = M.getDAG();
+ return DAG ? DAG->IsNegative(N) : false;
+ }
+};
+
+struct NonNegative_match {
+ template <typename MatchContext>
+ bool match(const MatchContext &M, SDValue N) const {
+ const SelectionDAG *DAG = M.getDAG();
+ return DAG ? DAG->IsNonNegative(N) : false;
+ }
+};
+
+struct StrictlyPositive_match {
+ template <typename MatchContext>
+ bool match(const MatchContext &M, SDValue N) const {
+ const SelectionDAG *DAG = M.getDAG();
+ return DAG ? DAG->IsStrictlyPositive(N) : false;
+ }
+};
+
+struct NonPositive_match {
+ template <typename MatchContext>
+ bool match(const MatchContext &M, SDValue N) const {
+ const SelectionDAG *DAG = M.getDAG();
+ return DAG ? DAG->IsNonPositive(N) : false;
+ }
+};
+
+struct NonZero_match {
+ template <typename MatchContext>
+ bool match(const MatchContext &M, SDValue N) const {
+ const SelectionDAG *DAG = M.getDAG();
+ return DAG ? DAG->IsNonZero(N) : false;
+ }
+};
+
struct Zero_match {
bool AllowUndefs;
@@ -1204,6 +1244,21 @@ struct AllOnes_match {
}
};
+inline Negative_match m_Negative() {
+ return Negative_match();
+}
+inline NonNegative_match m_NonNegative() {
+ return NonNegative_match();
+}
+inline StrictlyPositive_match m_StrictlyPositive() {
+ return StrictlyPositive_match();
+}
+inline NonPositive_match m_NonPositive() {
+ return NonPositive_match();
+}
+inline NonZero_match m_NonZero() {
+ return NonZero_match();
+}
inline Ones_match m_One(bool AllowUndefs = false) {
return Ones_match(AllowUndefs);
}
diff --git a/llvm/include/llvm/CodeGen/SelectionDAG.h b/llvm/include/llvm/CodeGen/SelectionDAG.h
index 4c7c9942fb8f6..4c760ee79cc41 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAG.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAG.h
@@ -2077,6 +2077,21 @@ class SelectionDAG {
/// floating-point value.
LLVM_ABI bool SignBitIsZeroFP(SDValue Op, unsigned Depth = 0) const;
+ /// Return true if Op is strictly positive FP or integer
+ LLVM_ABI bool IsStrictlyPositive(SDValue Op, unsigned Depth = 0) const;
+
+ /// Return true if Op is non positive FP or integer
+ LLVM_ABI bool IsNonPositive(SDValue Op, unsigned Depth = 0) const;
+
+ /// Return true if Op is a Negative FP or integer
+ LLVM_ABI bool IsNegative(SDValue Op, unsigned Depth = 0) const;
+
+ /// Return true if Op is non negative
+ LLVM_ABI bool IsNonNegative(SDValue Op, unsigned Depth = 0) const;
+
+ /// Return true if Op is non zero
+ LLVM_ABI bool IsNonZero(SDValue Op, unsigned Depth = 0) const;
+
/// Return true if 'Op & Mask' is known to be zero. We
/// use this predicate to simplify operations downstream. Op and Mask are
/// known to be the same type.
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 891f584cf0c3a..fd848633f9fdc 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -2834,6 +2834,50 @@ bool SelectionDAG::SignBitIsZeroFP(SDValue Op, unsigned Depth) const {
llvm_unreachable("covered opcode switch");
}
+bool SelectionDAG::IsNegative(SDValue Op, unsigned Depth) const {
+ if (Op.getValueType().isScalarInteger()
+ || Op.getValueType().isFloatingPoint()) {
+ KnownBits KB = computeKnownBits(Op);
+ return KB.isNegative();
+ }
+
+ return false;
+}
+
+bool SelectionDAG::IsNonNegative(SDValue Op, unsigned Depth) const {
+ if (Op.getValueType().isScalarInteger()
+ || Op.getValueType().isFloatingPoint()) {
+ KnownBits KB = computeKnownBits(Op);
+ return KB.isNonNegative();
+ }
+
+ return false;
+}
+
+bool SelectionDAG::IsStrictlyPositive(SDValue Op, unsigned Depth) const {
+ if (Op.getValueType().isScalarInteger()
+ || Op.getValueType().isFloatingPoint()) {
+ KnownBits KB = this->computeKnownBits(Op);
+ return KB.isStrictlyPositive();
+ }
+
+ return false;
+}
+
+bool SelectionDAG::IsNonPositive(SDValue Op, unsigned Depth) const {
+ return !IsStrictlyPositive(Op);
+}
+
+bool SelectionDAG::IsNonZero(SDValue Op, unsigned Depth) const {
+ if (Op.getValueType().isScalarInteger()
+ || Op.getValueType().isFloatingPoint()) {
+ KnownBits KB = computeKnownBits(Op);
+ return KB.isNonZero();
+ }
+
+ return false;
+}
+
/// MaskedValueIsZero - Return true if 'V & Mask' is known to be zero. We use
/// this predicate to simplify operations downstream. Mask is known to be zero
/// for bits that V cannot have.
diff --git a/llvm/unittests/CodeGen/SelectionDAGPatternMatchTest.cpp b/llvm/unittests/CodeGen/SelectionDAGPatternMatchTest.cpp
index 1afc034dd7b9e..8d2e8627f1d9f 100644
--- a/llvm/unittests/CodeGen/SelectionDAGPatternMatchTest.cpp
+++ b/llvm/unittests/CodeGen/SelectionDAGPatternMatchTest.cpp
@@ -556,6 +556,7 @@ TEST_F(SelectionDAGPatternMatchTest, matchConstants) {
SDValue ConstSplat = DAG->getSplat(VInt32VT, DL, Const3);
SDValue Zero = DAG->getConstant(0, DL, Int32VT);
SDValue One = DAG->getConstant(1, DL, Int32VT);
+ SDValue MinusOne = DAG->getConstant(APInt(Int32VT.getScalarSizeInBits(), -1, true), DL, Int32VT);
SDValue AllOnes = DAG->getConstant(APInt::getAllOnes(32), DL, Int32VT);
SDValue SetCC = DAG->getSetCC(DL, Int32VT, Arg0, Const3, ISD::SETULT);
@@ -575,6 +576,21 @@ TEST_F(SelectionDAGPatternMatchTest, matchConstants) {
EXPECT_TRUE(sd_match(One, DAG.get(), m_True()));
EXPECT_FALSE(sd_match(AllOnes, DAG.get(), m_True()));
+ EXPECT_TRUE (sd_match(MinusOne, DAG.get(), m_Negative()));
+ EXPECT_TRUE (sd_match(MinusOne, DAG.get(), m_NonZero()));
+ EXPECT_TRUE (sd_match(MinusOne, DAG.get(), m_NonPositive()));
+ EXPECT_FALSE(sd_match(MinusOne, DAG.get(), m_StrictlyPositive()));
+
+ EXPECT_FALSE(sd_match(Zero, DAG.get(), m_Negative()));
+ EXPECT_FALSE(sd_match(Zero, DAG.get(), m_NonZero()));
+ EXPECT_TRUE (sd_match(Zero, DAG.get(), m_NonPositive()));
+ EXPECT_FALSE(sd_match(Zero, DAG.get(), m_StrictlyPositive()));
+
+ EXPECT_FALSE(sd_match(One, DAG.get(), m_Negative()));
+ EXPECT_TRUE (sd_match(One, DAG.get(), m_NonZero()));
+ EXPECT_FALSE(sd_match(One, DAG.get(), m_NonPositive()));
+ EXPECT_TRUE (sd_match(One, DAG.get(), m_StrictlyPositive()));
+
ISD::CondCode CC;
EXPECT_TRUE(sd_match(
SetCC, m_Node(ISD::SETCC, m_Value(), m_Value(), m_CondCode(CC))));
|
| template <typename MatchContext> | ||
| bool match(const MatchContext &M, SDValue N) const { | ||
| const SelectionDAG *DAG = M.getDAG(); | ||
| return DAG ? DAG->IsNegative(N) : false; |
There was a problem hiding this comment.
I don't think we need new SelectionDAG helpers for these, try:
| return DAG ? DAG->IsNegative(N) : false; | |
| return DAG && DAG->computeKnownBits(N).IsNegative() ; |
There was a problem hiding this comment.
I was also checking whether the SDValue corresponds to a Scalar Integer or Floating point in the helper functions. Would that be necessary or this would suffice?
There was a problem hiding this comment.
Not necessary, we want this to work for vectors as well - computeKnownBits is your friend :)
| template <typename MatchContext> | ||
| bool match(const MatchContext &M, SDValue N) const { | ||
| const SelectionDAG *DAG = M.getDAG(); | ||
| return DAG ? DAG->IsNonNegative(N) : false; |
There was a problem hiding this comment.
| return DAG ? DAG->IsNonNegative(N) : false; | |
| return DAG && DAG->IsNonNegative(N); |
| template <typename MatchContext> | ||
| bool match(const MatchContext &M, SDValue N) const { | ||
| const SelectionDAG *DAG = M.getDAG(); | ||
| return DAG ? DAG->IsStrictlyPositive(N) : false; |
There was a problem hiding this comment.
| return DAG ? DAG->IsStrictlyPositive(N) : false; | |
| return DAG && DAG->IsStrictlyPositive(N); |
| template <typename MatchContext> | ||
| bool match(const MatchContext &M, SDValue N) const { | ||
| const SelectionDAG *DAG = M.getDAG(); | ||
| return DAG ? DAG->IsNonPositive(N) : false; |
There was a problem hiding this comment.
Might need to drop this one - not sure
There was a problem hiding this comment.
no you don't - we now have IsNonPositive :)
| template <typename MatchContext> | ||
| bool match(const MatchContext &M, SDValue N) const { | ||
| const SelectionDAG *DAG = M.getDAG(); | ||
| return DAG ? DAG->IsNonZero(N) : false; |
There was a problem hiding this comment.
| return DAG ? DAG->IsNonZero(N) : false; | |
| return DAG && DAG->IsNonZeroN); |
| @@ -1213,6 +1244,21 @@ struct AllOnes_match { | |||
| } | |||
| }; | |||
|
|
|||
| inline Negative_match m_Negative() { | |||
There was a problem hiding this comment.
We need captures for these, I need to be able to do the likes of:
sd_match(N, m_Sub(m_Value(X),m_Negative(m_Value(Y)))There was a problem hiding this comment.
Something like this should work (untested):
template <typename ValTy>
inline auto m_Negative(const ValTy &V) {
return m_AllOf(m_Negative(), V);
}
There was a problem hiding this comment.
Something like this should work (untested):
template <typename ValTy> inline auto m_Negative(const ValTy &V) { return m_AllOf(m_Negative(), V); }
Thanks! I had also worked out something similar but couldn't get it to work with Value_match() and Value_bind() both. This worked out great!
| LLVM_ABI bool IsNonNegative(SDValue Op, unsigned Depth = 0) const; | ||
|
|
||
| /// Return true if Op is non zero | ||
| LLVM_ABI bool IsNonZero(SDValue Op, unsigned Depth = 0) const; |
There was a problem hiding this comment.
Test with non-constants as well
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| template <typename MatchContext> | ||
| bool match(const MatchContext &M, SDValue N) const { | ||
| const SelectionDAG *DAG = M.getDAG(); | ||
| return DAG ? DAG->IsNonPositive(N) : false; |
There was a problem hiding this comment.
| return DAG ? DAG->IsNonPositive(N) : false; | |
| return DAG && DAG->IsNonPositive(N); |
…e/m_NonPositive/m_NonZero matchers
|
@RKSimon Sorry for the delay, I was a bit caught up with school and internship work, and couldn't devote much time to this issue / PR, the previous week. I have now made the changes requested and also rebased to make use of isNonPositive() helper in KnownBits. |
RKSimon
left a comment
There was a problem hiding this comment.
Please can you add a couple of tests case where DAG has not been set inside sd_match - just to ensure that we don't attempt deref a nullptr to DAG
67903a8 to
cfb843b
Compare
…e/m_NonPositive/m_NonZero matchers
|
Also, in the default configuration all tests in |
|
what targets are you building in cmake? |
Only X86 with RelWithDebInfo build type
|
|
Yes, you will need to add AARCH64 to your LLVM_TARGETS_TO_BUILD list as well - don't change the unit test triple |
I think I have already formatted my code, will check again. |
|
Take a look at the CI warning |
|
I checked again, and it did require some formatting 😅 Will pay attention to CI for any issues next time 👍 |
| SDValue SignBit = DAG->getConstant(0x80000000u, DL, Int32VT); | ||
| SDValue LSB = DAG->getConstant(0x00000001u, DL, Int32VT); | ||
| // ~SignBit | ||
| SDValue NotSignBit = DAG->getNode( |
There was a problem hiding this comment.
Probably a silly question; as a code reviewer, do you prefer amending existing commits and force pushing or making new commits?
There was a problem hiding this comment.
Extra commits are better - avoid rebase and force push whenever possible to retain comment history
There was a problem hiding this comment.
Extra commits are better - avoid rebase and force push whenever possible to retain comment history
|
@ak1932 If you're after a use case in DAGCombine - the HasKnownSameSign lambda inside visitIMINMAX might be a good option |
RKSimon
left a comment
There was a problem hiding this comment.
Missing a DAGCombine usage (if we can)
|
Yeah, the issue that I faced with finding a fold was that any fold I could think of was already performed using DAG.SignBitIsZero and such variants. I will check the lambda you mentioned 👍 |
auto HasKnownSameSign = [&](SDValue A, SDValue B) {
if (A.isUndef() || B.isUndef())
return true;
KnownBits KA = DAG.computeKnownBits(A);
if (!KA.isNonNegative() && !KA.isNegative())
return false;
KnownBits KB = DAG.computeKnownBits(B);
if (KA.isNonNegative())
return KB.isNonNegative();
return KB.isNegative();
};
if (HasKnownSameSign(N0, N1)) {Here, HasKnownSameSign lambda uses computeKnownBits once per value. To replace it with the new matchers we would use something like this if (sd_match(N, &DAG, m_BinOp(Opcode, m_NonPositive(), m_NonPositive()))
|| sd_match(N, &DAG, m_BinOp(Opcode, m_NonNegative(), m_NonNegative()))) {However, it would use computeKnownBits twice per value in non negative cases. |
RKSimon
left a comment
There was a problem hiding this comment.
LGTM - cheers - we'll add DAG combine coverage as it comes along
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/43/builds/37312 Here is the relevant piece of the build log for the reference |
…e/m_NonPositive/m_NonZero matchers (llvm#175191) Resolves llvm#174327
…e/m_NonPositive/m_NonZero matchers (llvm#175191) Resolves llvm#174327
wip: Still need to find a fold in DAGCombiner and add more extensive unit testing.
Resolves #174327