Skip to content
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

Merged
merged 3 commits into from
Oct 18, 2023
Merged

Conversation

christiankissig
Copy link
Contributor

@christiankissig christiankissig commented Sep 29, 2023

  • [Support] Add KnownBits::computeForSubBorrow
  • [CodeGen] Implement USUBC, USUBO_CARRY, and SSUBO_CARRY with KnownBits::computeForSubBorrow
  • [CodeGen] Compute unknown bits for Carry/Borrow for ADD/SUB
  • [CodeGen] Compute known bits of Carry/Borrow for UADDO, SADDO, USUBO, and SSUBO

Fixes #65893

Copy link
Contributor

@nikic nikic left a 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.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp Outdated Show resolved Hide resolved
@goldsteinn
Copy link
Contributor

Might be nice to add a test where this actually shows up in CodeGen.

@christiankissig
Copy link
Contributor Author

Might be nice to add a test where this actually shows up in CodeGen.

+1 Will look to add it with this change.

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick

llvm/lib/Support/KnownBits.cpp Outdated Show resolved Hide resolved
@dlumma
Copy link

dlumma commented Oct 10, 2023

It seems like this PR is ready to land. Any reason why it has not been integrated @christiankissig ?

@christiankissig
Copy link
Contributor Author

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.

@christiankissig christiankissig force-pushed the main branch 2 times, most recently from 98a1cb1 to dd7e9f8 Compare October 10, 2023 20:41
@github-actions
Copy link

github-actions bot commented Oct 10, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@christiankissig christiankissig force-pushed the main branch 2 times, most recently from bf8c8b0 to a445248 Compare October 11, 2023 06:11
@christiankissig
Copy link
Contributor Author

I've addressed final comments, rebased the changes and removed merge commits, reran all tests, but seem to lack permissions

> git push -f origin main
ERROR: Permission to llvm/llvm-project.git denied to christiankissig.

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - cheers

@goldsteinn
Copy link
Contributor

Still think a test where this actually comes up in CodeGen somewhere would be nice, but the code all looks good.

@christiankissig
Copy link
Contributor Author

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?

@RKSimon
Copy link
Collaborator

RKSimon commented Oct 16, 2023

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).

@goldsteinn
Copy link
Contributor

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?

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.

@github-actions
Copy link

github-actions bot commented Oct 18, 2023

✅ With the latest revision this PR passed the Python code formatter.

christiankissig and others added 3 commits October 18, 2023 07:56
Implements computeForSubBorrow as alias for computeforAddCarry. Borrow is expected to be 1-bit wide.

Adds exhaustive unit test.
… 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]>
Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RKSimon
Copy link
Collaborator

RKSimon commented Oct 18, 2023

@christiankissig Do you have commit access yet or shall I squash + merge it?

@christiankissig
Copy link
Contributor Author

I don't have access yet. Please merge. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:SelectionDAG SelectionDAGISel as well llvm:support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Support] Add KnownBits::computeForSubBorrow
7 participants