[DAG] isKnownToBeAPowerOfTwo - Power of 2 value is known to be power of 2 after BSWAP/BITREVERSE - #182207
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. |
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-llvm-selectiondag Author: Manuel Dun (manueldun) ChangesThis is my first pr specifically for llvm Full diff: https://github.com/llvm/llvm-project/pull/182207.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 581553d41cb6d..b24468fbc42a4 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -4762,6 +4762,10 @@ bool SelectionDAG::isKnownToBeAPowerOfTwo(SDValue Val,
isKnownToBeAPowerOfTwo(Val.getOperand(0), /*OrZero=*/false, Depth + 1))
return true;
break;
+ case ISD::BSWAP:
+ case ISD::BITREVERSE:
+ isKnownToBeAPowerOfTwo(Val.getOperand(0), /*OrZero=*/false, Depth + 1);
+ break;
}
// More could be done here, though the above checks are enough
diff --git a/llvm/unittests/Target/AArch64/AArch64SelectionDAGTest.cpp b/llvm/unittests/Target/AArch64/AArch64SelectionDAGTest.cpp
index b2b8dfb0c21fc..0fa4b9dcbcbf5 100644
--- a/llvm/unittests/Target/AArch64/AArch64SelectionDAGTest.cpp
+++ b/llvm/unittests/Target/AArch64/AArch64SelectionDAGTest.cpp
@@ -914,6 +914,17 @@ TEST_F(AArch64SelectionDAGTest, KnownToBeAPowerOfTwo_Constants) {
EXPECT_TRUE(DAG->isKnownToBeAPowerOfTwo(SplatBig, /*OrZero=*/true));
}
+TEST_F(AArch64SelectionDAGTest, KnownToBeAPowerOfTwo_BSWAP_BITREVERSE) {
+ SDLoc Loc;
+ auto Cst4 = DAG->getConstant(4, Loc, MVT::i32);
+ auto Int32Vt = MVT::i32;
+ auto BSwapOp = DAG->getNode(ISD::BSWAP, Loc, Int32Vt, Cst4);
+ EXPECT_TRUE(DAG->isKnownToBeAPowerOfTwo(BSwapOp, false));
+
+ auto BReverseOp = DAG->getNode(ISD::BSWAP, Loc, Int32Vt, BSwapOp);
+ EXPECT_TRUE(DAG->isKnownToBeAPowerOfTwo(BReverseOp, false));
+}
+
TEST_F(AArch64SelectionDAGTest, KnownToBeAPowerOfTwo_Select) {
SDLoc Loc;
auto Cst0 = DAG->getConstant(0, Loc, MVT::i32);
|
| auto BSwapOp = DAG->getNode(ISD::BSWAP, Loc, Int32Vt, Cst4); | ||
| EXPECT_TRUE(DAG->isKnownToBeAPowerOfTwo(BSwapOp, false)); | ||
|
|
||
| auto BReverseOp = DAG->getNode(ISD::BSWAP, Loc, Int32Vt, BSwapOp); |
There was a problem hiding this comment.
Forgot to change the opcode to ISD::BITREVERSE?
There was a problem hiding this comment.
Still need vector tests (with and without DemandedElts) and OrZero handling tests
| @@ -4762,6 +4762,10 @@ bool SelectionDAG::isKnownToBeAPowerOfTwo(SDValue Val, | |||
| isKnownToBeAPowerOfTwo(Val.getOperand(0), /*OrZero=*/false, Depth + 1)) | |||
| return true; | |||
| break; | |||
| case ISD::BSWAP: | |||
| case ISD::BITREVERSE: | |||
| isKnownToBeAPowerOfTwo(Val.getOperand(0), /*OrZero=*/false, Depth + 1); | |||
There was a problem hiding this comment.
Missing return, DemandedElts and OrZero handling
| auto BSwapOp = DAG->getNode(ISD::BSWAP, Loc, Int32Vt, Cst4); | ||
| EXPECT_TRUE(DAG->isKnownToBeAPowerOfTwo(BSwapOp, false)); | ||
|
|
||
| auto BReverseOp = DAG->getNode(ISD::BSWAP, Loc, Int32Vt, BSwapOp); |
There was a problem hiding this comment.
Still need vector tests (with and without DemandedElts) and OrZero handling tests
| @@ -914,6 +914,17 @@ TEST_F(AArch64SelectionDAGTest, KnownToBeAPowerOfTwo_Constants) { | |||
| EXPECT_TRUE(DAG->isKnownToBeAPowerOfTwo(SplatBig, /*OrZero=*/true)); | |||
| } | |||
|
|
|||
| TEST_F(AArch64SelectionDAGTest, KnownToBeAPowerOfTwo_BSWAP_BITREVERSE) { | |||
| SDLoc Loc; | |||
| auto Cst4 = DAG->getConstant(4, Loc, MVT::i32); | |||
There was a problem hiding this comment.
can I ask why no auto? are we talking about auto type deduction?
There was a problem hiding this comment.
The style guide recommends avoiding them except for explicit casts or where the type is obvious:
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
| case ISD::BSWAP: | ||
| case ISD::BITREVERSE: |
There was a problem hiding this comment.
Merge this into the ROTL/ROTR case above, which could also pass through the orZero flag?
7618fd8 to
dc45e74
Compare
|
How about now? waiting for feedback |
|
|
||
| SDLoc Loc; | ||
| SDValue Cst4 = DAG->getConstant(4, Loc, MVT::i32); | ||
| SDValue BSwapOp = DAG->getNode(ISD::BSWAP, Loc, MVT::i32, Cst4); |
There was a problem hiding this comment.
Doesn't creating a BSWAP with constant inputs constant fold to a new constant inside getNode? Meaning the call to isKnownToBeAPowerOfTwo is being called on a constant, not on a BSWAP node?
I copied this block locally and it passes without the change to SelectionDAG.cpp.
There was a problem hiding this comment.
To address this I tried to use an operator that cant be folded to a constant (eg. bitcast) and use the result to a bswap, but isKnownToBeAPowerOfTwo does not return true with and without the changes.
This is what I tried:
SDLoc Loc;
SDValue Cst4 = DAG->getConstant(4, Loc, MVT::i32);
EVT InTypes[] = {MVT::i32,MVT::i32};
SDVTList IntTypes{InTypes,2};
SDValue CastOp = DAG->getNode(ISD::BITCAST,Loc,IntTypes,Cst4);
SDValue BSwapOp = DAG->getNode(ISD::BSWAP, Loc, MVT::i32, CastOp);
EXPECT_TRUE(DAG->isKnownToBeAPowerOfTwo(BSwapOp, /*OrZero=*/false));
As you can see, I cast an i32 to an i32 (a noop)
I wonder if something similar to this aproach would work.
There was a problem hiding this comment.
You will need to use another isKnownToBeAPowerOfTwo pattern inside to handle the OrZero.
Try - SELECT/VSELECT - take part of a test from KnownToBeAPowerOfTwo_Select and wrap the BSWAP/BITREVERSE nodes around them. Should make it easy to do OrZero=true/false and DemandedElts tests
|
|
||
| SDLoc Loc; | ||
| SDValue Cst4 = DAG->getConstant(4, Loc, MVT::i32); | ||
| SDValue BSwapOp = DAG->getNode(ISD::BSWAP, Loc, MVT::i32, Cst4); |
There was a problem hiding this comment.
You will need to use another isKnownToBeAPowerOfTwo pattern inside to handle the OrZero.
Try - SELECT/VSELECT - take part of a test from KnownToBeAPowerOfTwo_Select and wrap the BSWAP/BITREVERSE nodes around them. Should make it easy to do OrZero=true/false and DemandedElts tests
dc45e74 to
6df9786
Compare
| @@ -914,6 +914,113 @@ TEST_F(AArch64SelectionDAGTest, KnownToBeAPowerOfTwo_Constants) { | |||
| EXPECT_TRUE(DAG->isKnownToBeAPowerOfTwo(SplatBig, /*OrZero=*/true)); | |||
| } | |||
|
|
|||
| TEST_F(AArch64SelectionDAGTest, KnownToBeAPowerOfTwo_BSWAP_BITREVERSE) { | |||
There was a problem hiding this comment.
We can instead add this to KnownToBeAPowerOfTwo_Select (rename KnownToBeAPowerOfTwo_Ops?)
And just wraps some of the existing interesting VSelect values with bitreverse/bswap and repeat the same true/false test
This will scale much better than repeating so much duplicated setup code/.
6df9786 to
869886c
Compare
|
does the merge commit need to be handle in some way? (like squashing?) |
|
Merge is much preferred over rebase as rebase can lose review comments |
747ddc5 to
869886c
Compare
|
Sorry, it is my first time resolving a merge conflict, how is the correct way to do so? |
|
Sorry for squashing commits, I assumed it was the way to go, let me create the tests and resolve conflicts |
34f2d7c to
b069700
Compare
|
I will try to not do more rebasing from here |
np - re-request a review when you're ready |
| %and = and i32 %x, %d | ||
| %r = icmp eq i32 %and, %d | ||
| ret i1 %r | ||
| } |
There was a problem hiding this comment.
don't remove existing test coverage
There was a problem hiding this comment.
sorry, I removed the wrong test, my bad
| %x_add_y = add i32 %x, %rot_y | ||
| %r = and i32 %x_add_y, %rot_y | ||
| ret i32 %r | ||
| } |
There was a problem hiding this comment.
remove this new rot test coverage
| %x_add_y = add i32 %x, %rot_y | ||
| %r = and i32 %x_add_y, %rot_y | ||
| ret i32 %r | ||
| } |
There was a problem hiding this comment.
remove this new rot test coverage
6b5d997 to
45cc6ca
Compare
| %sel = select i1 %cmp, i32 4, i32 8 | ||
| %swap = call i32 @llvm.bswap.i32(i32 %sel) | ||
| %rev = call i32 @llvm.bitreverse.i32(i32 %sel) | ||
| %res = urem i32 %a1, %swap |
| ; CHECK-NEXT: retq | ||
| %cmp = icmp sgt <4 x i32> zeroinitializer, %a0 | ||
| %sel = select <4 x i1> %cmp, <4 x i32> <i32 4, i32 2, i32 1, i32 0>, <4 x i32> <i32 8, i32 4, i32 2, i32 -1> | ||
| %swap = call <4 x i32> @llvm.bswap.v4i32(<4 x i32> %sel) |
There was a problem hiding this comment.
| %swap = call <4 x i32> @llvm.bswap.v4i32(<4 x i32> %sel) | |
| %rev = call <4 x i32> @llvm.bitreverse.v4i32(<4 x i32> %sel) |
|
@manueldun Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
…of 2 after BSWAP/BITREVERSE (llvm#182207) This is my first pr specifically for llvm Fixes llvm#181657 Allows the isKnownToBeAPowerOfTwo function to handle BSWAP and BITREVERSE dag operations.
This is my first pr specifically for llvm
Fixes #181657
Allows the isKnownToBeAPowerOfTwo function to handle BSWAP and BITREVERSE dag operations.
Any feedback is welcome.