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

More control barrier tests #579

Merged
merged 6 commits into from
Nov 30, 2023
Merged

More control barrier tests #579

merged 6 commits into from
Nov 30, 2023

Conversation

hernanponcedeleon
Copy link
Owner

@tonghaining can you take a look?

@@ -961,7 +961,9 @@ public Knowledge visitSyncBarrier(Relation sync_bar) {
List<FenceWithId> fenceEvents = program.getThreadEvents(FenceWithId.class);
for (FenceWithId e1 : fenceEvents) {
for (FenceWithId e2 : fenceEvents) {
if(exec.areMutuallyExclusive(e1, e2)) {
// “A bar.sync or bar.red or bar.arrive operation synchronizes with a bar.sync
Copy link
Contributor

Choose a reason for hiding this comment

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

what is bar.red?

Copy link
Owner Author

Choose a reason for hiding this comment

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

// Use bar.red.and to compare results across the entire CTA:
setp.eq.u32 p,r1,r2; // p is True if r1==r2
bar.cta.red.and.pred r3,1,p; // r3=AND(p) forall threads in CTA

// Use bar.red.popc to compute the size of a group of threads
// that have a specific condition True:
setp.eq.u32 p,r1,r2; // p is True if r1==r2
bar.cta.red.popc.u32 r3,1,p; // r3=SUM(p) forall threads in CTA

We don't currently support those instructions, but I just copied the comment above from the documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I think the current implementation support (bar.sync, bar.sync) and (bar.arrive, bar.sync).
Shall we also update the comments on two ptx cat files about the sync_barrier relation?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't think we need that level of detail in the cat file. E.g. we do not specify either all the cases covered by sr.

}
P0@sg 0, wg 0, qf 0 | P1@sg 0,wg 0, qf 0 ;
ld.atom.wg.sc0 r0, x | cbar.wg.semsc0 0 ;
cbar.wg.semsc0 0 | st.atom.wg.sc0 x, 1 ;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to be cbar.wg 0 and also for the cbar in the second thread.
Storage class semantic is used for acq/rel in the original method.
It might be relevant to this part of the model?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good catch. The problem is that the alloy model specifies that ACQ + REL = SEMSC0 + SEMSC1, but our spriv-check.cat only checked half of the equality (i.e. one inclusion).

I fixed the cat file (which also required fixing the propagation of tags in the Vulkan visitor because we should not propagate semscX to the read event coming from a rmw.rel instruction).

There are also a lot of CADP test failing but it might be easier to fix the generation script than going one by one over the tests. @tonghaining can you do this?

@tonghaining
Copy link
Contributor

LGTM

@hernanponcedeleon hernanponcedeleon merged commit 3efc7bc into development Nov 30, 2023
1 check passed
@hernanponcedeleon hernanponcedeleon deleted the ptx.arrive branch November 30, 2023 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants