Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Soundness bugfix for barrier<thread_scope_block> on sm_70 #300

Merged
merged 1 commit into from
Sep 15, 2022

Conversation

gonzalobg
Copy link
Collaborator

For sm_70, barrier arrive has an optimization to "coalesce" all arrives with the same update to the same barrier into a single update performed by a "leader" thread.

This optimization is missing a release fence to establish cummulativity between all coalesced threads and the leader, before the leader performs the update.

@gonzalobg gonzalobg requested a review from griwes August 10, 2022 13:34
@gonzalobg
Copy link
Collaborator Author

@daniellustig could maybe review?

unsigned int __active = __activeA & __activeB;
int __inc = __popc(__active) * __update;

unsigned __laneid;
asm volatile ("mov.u32 %0, %laneid;" : "=r"(__laneid));
asm ("mov.u32 %0, %laneid;" : "=r"(__laneid));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you removing the volatile here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because it is not necessary. Two "calls" to this assembly instruction from this same thread always "return" the same value, so eliding one is a valid transformation for the compiler to do.

This assembly statement does not modify any memory, so it does not need a "memory" clobber either.

@daniellustig
Copy link
Contributor

The cumulativity fix seems reasonable to me

@gonzalobg
Copy link
Collaborator Author

@wmaxey ?

Copy link
Contributor

@davedsth davedsth left a comment

Choose a reason for hiding this comment

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

Looks ok, we should think of a refactor reinterpret_caststd::uintptr_t(&__barrier) and __match_any_sync

@wmaxey wmaxey merged commit b67d355 into NVIDIA:main Sep 15, 2022
@wmaxey
Copy link
Member

wmaxey commented Sep 15, 2022

Thanks for review, David. Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants