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.

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

[CIR][CIRGen] Support for builtin __atomic_thread_fence #1287

Merged
merged 2 commits into from
Feb 12, 2025
Merged

[CIR][CIRGen] Support for builtin __atomic_thread_fence #1287

merged 2 commits into from
Feb 12, 2025

Conversation

Rajveer100
Copy link
Contributor

@Rajveer100 Rajveer100 commented Jan 17, 2025 •
edited by bcardosolopes
Loading

Fix #1274

Implements atomic thread fence synchronization primitive corresponding to atomic.thread_fence CIR.

@Rajveer100
Copy link
Contributor Author

I have just created the ODS definition at the moment to be sure if this is the way to go.

@Rajveer100 Rajveer100 mentioned this pull request Jan 17, 2025
Closed
4 tasks
@Rajveer100 Rajveer100 force-pushed the atomic-thread_signal-fence branch from 969faf7 to c2cc1a9 Compare January 19, 2025 12:00
bcardosolopes
bcardosolopes reviewed Jan 21, 2025
View reviewed changes
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Initial step looks good, some inline comments. Note that this PR should come with CIRGen and LLVM lowering support, I suggest you first handle a very simple case of the fence and mark all paths not currently support with llvm_unrecheable("NYI"), so that you can go about working in incremental PRs to complete this.

@bcardosolopes bcardosolopes changed the title [CIRGen] Task 1: Support for builtin __atomic_thread_fence [CIR][CIRGen] Support for builtin __atomic_thread_fence Jan 21, 2025
@Rajveer100 Rajveer100 force-pushed the atomic-thread_signal-fence branch from c2cc1a9 to e8758ac Compare January 26, 2025 10:50
@bcardosolopes
Copy link
Member

We just went over a rebase against upstream, apologies for the churn but please update your branch for this PR and force-push!

@Rajveer100 Rajveer100 force-pushed the atomic-thread_signal-fence branch from e8758ac to ca75a57 Compare January 28, 2025 11:23
@ghehg ghehg self-requested a review January 28, 2025 20:57
bcardosolopes
bcardosolopes requested changes Jan 29, 2025
View reviewed changes
ghehg
ghehg reviewed Feb 3, 2025
View reviewed changes
@Rajveer100 Rajveer100 force-pushed the atomic-thread_signal-fence branch 2 times, most recently from 6425312 to 97fb189 Compare February 6, 2025 12:02
@Rajveer100
Copy link
Contributor Author

Rajveer100 commented Feb 6, 2025 •
edited
Loading

Do we need a new definition for AtomicSyncScope in LLVMEnums.td like there's one for ordering and then retrieve conversion using getLLVM...()?

@Rajveer100 Rajveer100 requested a review from ghehg February 6, 2025 13:25
@ghehg
Copy link
Collaborator

ghehg commented Feb 6, 2025 •
edited
Loading

Do we need a new definition for AtomicSyncScope in LLVMEnums.td like there's one for ordering and then retrieve conversion using getLLVM...()?

That is kinda of territory of LLVM dialect. Currently llvm.fenceop uses stringattr to represent syncscope, and I assume it works meaning it lowers to LLVM IR without problem if we set it with right string value from clangir lowering to LLVM (dialect)

@Rajveer100 Rajveer100 force-pushed the atomic-thread_signal-fence branch from 97fb189 to 39bfe2b Compare February 7, 2025 08:13
@Rajveer100 Rajveer100 requested a review from ghehg February 7, 2025 11:33
@Rajveer100
Copy link
Contributor Author

From my sight I can see the following nits:

  • Better variable naming in the test suite
  • Finish the description to show example usage of the op

@Rajveer100 Rajveer100 force-pushed the atomic-thread_signal-fence branch 2 times, most recently from 1a626c7 to 83b33a7 Compare February 8, 2025 12:01
bcardosolopes
bcardosolopes reviewed Feb 10, 2025
View reviewed changes
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Mostly good, few nits and ready to go

@Rajveer100 Rajveer100 force-pushed the atomic-thread_signal-fence branch from 83b33a7 to 8f81369 Compare February 10, 2025 11:28
@Rajveer100 Rajveer100 force-pushed the atomic-thread_signal-fence branch from 8f81369 to 35d7898 Compare February 11, 2025 08:30
@ghehg
Copy link
Collaborator

ghehg commented Feb 11, 2025

LGMT, too

@Rajveer100 Rajveer100 force-pushed the atomic-thread_signal-fence branch from 35d7898 to 9699710 Compare February 12, 2025 08:35
@Rajveer100
[CIR][CIRGen] Support for builtin __atomic_thread_fence
9e8d54f 
Resolves llvm#1274

Implements atomic thread fence synchronization primitive
corresponding to `atomic.thread_fence` CIR.
@Rajveer100 Rajveer100 force-pushed the atomic-thread_signal-fence branch 2 times, most recently from a191e33 to 0a24062 Compare February 12, 2025 10:09
@Rajveer100 Rajveer100 force-pushed the atomic-thread_signal-fence branch from 0a24062 to 0ed0cb6 Compare February 12, 2025 10:46
@bcardosolopes bcardosolopes merged commit 6dd6d82 into llvm:main Feb 12, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcardosolopes bcardosolopes bcardosolopes approved these changes

@lanza lanza Awaiting requested review from lanza lanza is a code owner

@ghehg ghehg Awaiting requested review from ghehg

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

Successfully merging this pull request may close these issues.

CIRGen support for *_atomic_{thread,signal}_fence
3 participants
@Rajveer100 @bcardosolopes @ghehg