-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[DAG] SDPatternMatch - Fix m_Reassociatable mismatching #170061
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
Changes from 4 commits
ab6d06a
d989ca1
f049fd9
15adee2
d9833cf
5c4ba06
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -842,6 +842,28 @@ TEST_F(SelectionDAGPatternMatchTest, matchReassociatableOp) { | |
| EXPECT_TRUE(sd_match( | ||
| MUL, m_ReassociatableMul(m_Value(), m_Value(), m_Value(), m_Value()))); | ||
|
|
||
| // (Op0 + Op1) + Op0 binds correctly, allowing commutation | ||
| SDValue ADD010 = DAG->getNode(ISD::ADD, DL, Int32VT, ADD01, Op0); | ||
| SDValue A, B; | ||
| EXPECT_TRUE(sd_match( | ||
| ADD010, m_ReassociatableAdd(m_Value(A), m_Value(B), m_Deferred(A)))); | ||
| EXPECT_EQ(Op0, A); | ||
| EXPECT_EQ(Op1, B); | ||
| EXPECT_NE(A, B); | ||
mshockwave marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| A.setNode(nullptr); | ||
| B.setNode(nullptr); | ||
| EXPECT_TRUE(sd_match( | ||
| ADD010, m_ReassociatableAdd(m_Value(A), m_Value(B), m_Deferred(B)))); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem I was often seeing wasn't that m_ReassociatableAdd etc. didn't return true - it was that the values of A + B weren't correct - please can you add additional checks that A and B are correctly initialized to the correct ADD01/Op0 pairs?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, alright. Just added tests for this in the newest commit. |
||
| EXPECT_EQ(Op0, B); | ||
| EXPECT_EQ(Op1, A); | ||
| EXPECT_NE(A, B); | ||
|
|
||
| A.setNode(nullptr); | ||
| B.setNode(nullptr); | ||
| EXPECT_FALSE(sd_match( | ||
| ADD010, m_ReassociatableAdd(m_Value(A), m_Deferred(A), m_Deferred(A)))); | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you add some negative test cases? That is, showing that this pattern does not match when it should not.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just added a few more negative tests in response to another review, but they were all related to the binds done on the 2 positive test cases, e.g. to check m_Deferred(A) does not match with Op1 in the first case. I don't know if that meets what you asked for, though. Do you want me to add completely new negative test cases, alongside the current trivial one? |
||
| // Op0 * (Op1 * (Op2 * Op3)) | ||
| SDValue MUL123 = DAG->getNode(ISD::MUL, DL, Int32VT, Op1, MUL23); | ||
| SDValue MUL0123 = DAG->getNode(ISD::MUL, DL, Int32VT, Op0, MUL123); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.