Skip to content

[LangRef][IR] Add 3-way compare intrinsics llvm.scmp/llvm.ucmp #83227

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 43 commits into from
Mar 18, 2024
Merged

[LangRef][IR] Add 3-way compare intrinsics llvm.scmp/llvm.ucmp #83227

merged 43 commits into from
Mar 18, 2024

Conversation

miguelraz
Copy link
Contributor

@miguelraz miguelraz commented Feb 28, 2024 •
edited by nikic
Loading

This PR adds the [us]cmp intrinsics to the LangRef, Intrinsics.td and some tests to the IRVerifier.

RFC: https://discourse.llvm.org/t/rfc-add-3-way-comparison-intrinsics/76685

@miguelraz
Copy link
Contributor Author

WOW, format on save really screwed up my day here. I'll fix it tomorrow and learn about tablegen. Sorry for the noise.

@dtcxzyw
Copy link
Member

dtcxzyw commented Feb 29, 2024

Please split this PR into separate ones.

@miguelraz
Copy link
Contributor Author

@dtcxzyw gladly! I think it makes sense to split it after completing Stage 2 and landing support in SelectionDAG for the ThreeWayCmp. Does that seem reasonable?

@dtcxzyw dtcxzyw requested review from nikic, RKSimon and topperc February 29, 2024 12:04
@nikic
Copy link
Contributor

nikic commented Feb 29, 2024

@dtcxzyw gladly! I think it makes sense to split it after completing Stage 2 and landing support in SelectionDAG for the ThreeWayCmp. Does that seem reasonable?

I'd recommend making the first patch the change to LangRef, Intrinsics.td and (if necessary) the IR Verifier only, and then follow up with the actual SDAG implementation later. This is the critical-path change that everything else depends on, and LangRef changes tend to involve a lot of bikeshedding.

@miguelraz
Copy link
Contributor Author

That seems very reasonable then. Thanks!

@miguelraz miguelraz force-pushed the spaceship-intrinsic branch from 88593fa to 34c0531 Compare March 1, 2024 02:50
@miguelraz
Copy link
Contributor Author

@nikic Alright, first proper draft is up.

Areas for improvement:

  1. I didn't know how to bring up the return type for sthreecmp or uthreecmp to be iM bits in the LangRef. I welcome suggestions.
  2. Tablegen is new and scary. I searched for similar optims and I added the IntrSpeculatable to the block.
  3. I don't have a strong stance on the naming for sthreecmp. Bikeshed away.

@miguelraz miguelraz changed the title Spaceship intrinsic for GSoC 2024 Add 3 way compare <=> integer intrinsics to Langref Mar 1, 2024
nikic
nikic reviewed Mar 1, 2024
View reviewed changes
dc03-work
dc03-work reviewed Mar 2, 2024
View reviewed changes
@miguelraz miguelraz mentioned this pull request Mar 10, 2024
Closed
@tschuett
Copy link

For reference, a GlobalIsel PR that introduced new named opcodes and touched the IRTranslator.
#75086

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 11, 2024 •
edited
Loading

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

scottmcm
scottmcm reviewed Mar 11, 2024
View reviewed changes
scottmcm
scottmcm reviewed Mar 11, 2024
View reviewed changes
nikic
nikic reviewed Mar 11, 2024
View reviewed changes
@miguelraz
Copy link
Contributor Author

@dc03-work , oh, that git clang-format is super useful, thanks a lot! I had to turn off the formatter on VSCode after it borked an entire file the first time I was working on it and was just doing it mechanically.

I'll try to add it as a prehook commit hook eventually, thanks for pointing that out!

@miguelraz
Copy link
Contributor Author

@nikic if we're fine with adding/correcting the unit tests failing for now, this should be good to merge.

@nikic nikic changed the title [LangRef][IR] Add 3 way compare <=> integer intrinsics [LangRef][IR] Add 3-way compare intrinsics llvm.scmp/llvm.ucmp Mar 14, 2024
nikic
nikic approved these changes Mar 15, 2024
View reviewed changes
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.

LGTM

@miguelraz
Copy link
Contributor Author

@nikic fwiw Linux builders have passed but we're still waiting on a windows builder -_-

@miguelraz
Copy link
Contributor Author

miguelraz commented Mar 15, 2024 •
edited
Loading

ping for @scottmcm, @dc03-work, @topperc, and @RKSimon for review in case the other notifications drowned it out.

dc03-work
dc03-work approved these changes Mar 15, 2024
View reviewed changes
Copy link
Contributor

@dc03-work dc03-work left a comment

Choose a reason for hiding this comment

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

LGTM with a couple minor nits.

@scottmcm
Copy link

Thanks for the ping! I'm just the peanut gallery here; I'm no gate at all. Looks reasonable, though.

topperc
topperc approved these changes Mar 15, 2024
View reviewed changes
Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

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

LGTM

@nikic nikic merged commit 276847a into llvm:main Mar 18, 2024
@miguelraz miguelraz mentioned this pull request Mar 19, 2024
Draft
11 tasks
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
@miguelraz @chencha3
[LangRef][IR] Add 3-way compare intrinsics llvm.scmp/llvm.ucmp (llvm#…
13d7a80 
…83227)

This PR adds the `[us]cmp` intrinsics to the LangRef, `Intrinsics.td`
and some tests to the IRVerifier.

RFC: https://discourse.llvm.org/t/rfc-add-3-way-comparison-intrinsics/76685
tschuett added a commit to tschuett/llvm-project that referenced this pull request Jul 15, 2024
@tschuett
[GlobalIsel] Add G_SCMP and G_UCMP instructions
001bf34 
llvm#83227
tschuett added a commit to tschuett/llvm-project that referenced this pull request Jul 17, 2024
@tschuett
[GlobalIsel] Add G_SCMP and G_UCMP instructions
5b64f0a 
llvm#83227
tschuett added a commit that referenced this pull request Jul 18, 2024
[GlobalIsel] Add G_SCMP and G_UCMP instructions (#98894)
1cc1072 
#83227
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
@yuxuanchen1997
[GlobalIsel] Add G_SCMP and G_UCMP instructions (#98894)
311d849 
#83227
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottmcm scottmcm scottmcm left review comments

@nikic nikic nikic approved these changes

@topperc topperc topperc approved these changes

@dc03-work dc03-work dc03-work approved these changes

@RKSimon RKSimon Awaiting requested review from RKSimon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@miguelraz @dtcxzyw @nikic @tschuett @dc03-work @scottmcm @topperc