-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
[Support] Add KnownBits::computeForSubBorrow #67788
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me.
Might be nice to add a test where this actually shows up in CodeGen. |
+1 Will look to add it with this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick
It seems like this PR is ready to land. Any reason why it has not been integrated @christiankissig ? |
The PR had not been merged to add a CodeGen regression test as I said I would add in #67788 (comment) . Having looked into adding one, I realize this may need more work, compiler intrinsics for add.sub with carry. This is taking too long and may exceed the scope of the issue, so will break out into a separate piece. I've now made the final requested changes, and this PR is ready to merge. |
98a1cb1
to
dd7e9f8
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
bf8c8b0
to
a445248
Compare
I've addressed final comments, rebased the changes and removed merge commits, reran all tests, but seem to lack permissions
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - cheers
Still think a test where this actually comes up in CodeGen somewhere would be nice, but the code all looks good. |
@goldsteinn I agree, this would be nice to have, but I wonder if it's worth blocking this PR on. Based on what I understand - this is my first contribution to LLVM - , we're still missing compiler intrinsics and Target lowering code, before we can test the compiler optimization on LLIR, which would all be out of scope of the ticket this is addressing. I've added regression tests ensuring that the KnownBits calculation works, so I don't think we're particularly lacking test coverage here. What are your concerns specifically? |
Happy for this to go in as is (with the UADDO_CARRY -> USUBO_CARRY typo fix), this is still my preferred option. But also happy if there's still resistance to drop the SelectionDAG changes and we just get the KnownBits helper added for now (still with exhaustive tests). |
Sorry missed this, I didn't mean for the concern to be blocking this. If its not really doable that okay and this change LGTM. |
✅ With the latest revision this PR passed the Python code formatter. |
Implements computeForSubBorrow as alias for computeforAddCarry. Borrow is expected to be 1-bit wide. Adds exhaustive unit test.
…s::computeForSubBorrow
… and SSUBO Computes known bits for carry/borrow for UADDO_CARRY and USUBO_CARRY Operations. Carry/borrow are truncated to bit width 1. Adds computeKnownBits for Carry/Borrow for UADDO, SADDO, USUBO, and SSUBO. Carry over is expected to be of bit width 1. Adds unit tests for UADDO_CARRY and USUBO_CARRY. Adds a unit test for UADDO_CARRY and USUBO_CARRY. Co-authored-by: Shafik Yaghmour <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@christiankissig Do you have commit access yet or shall I squash + merge it? |
I don't have access yet. Please merge. Thank you! |
Fixes #65893