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

Fix default configuration for litmus tests #720

Merged
merged 2 commits into from
Aug 26, 2024
Merged

Conversation

hernanponcedeleon
Copy link
Owner

No description provided.

@natgavrilenko
Copy link
Collaborator

Please add some title to the merge request. Now it is just "717".

properties);
throw new IllegalArgumentException(error);
logger.warn(warn);
properties = EnumSet.of(Property.PROGRAM_SPEC);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is fine as a quick fix, but in the long term we probably want to have a more consistent solution. For example, does it make sense to verify data races for litmus tests at all? There will be many races for tests like MP.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why should litmus tests have many races? Hardware models lack a notion of race. Language models like (R)C11 need to use proper atomics for MP which also ensure that no data races occur.
Is this different for GPUs?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Language models like (R)C11 need to use proper atomics for MP which also ensure that no data races occur.

This is not true, even for (R)C11. In the usual MP litmus tests (i.e. without having a spinloop), marking the accesses to flag as release-acquire, but keeping the accesses to data as plain, is enough to prevent the reordering related to the exists. However, you will have races in any execution where the acquire-load does not read from the release-store.

We probably want to have a more consistent solution.

Checking races in litmus tests is useful to understand the definition of data race specific to the model. If the user does not want this, they can simply choose a different/single property.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Language models like (R)C11 need to use proper atomics for MP which also ensure that no data races occur.

This is not true, even for (R)C11. In the usual MP litmus tests (i.e. without having a spinloop), marking the accesses to flag as release-acquire, but keeping the accesses to data as plain, is enough to prevent the reordering related to the exists. However, you will have races in any execution where the acquire-load does not read from the release-store.

Fair enough. Executions that fail the exists clause may indeed be racy.

@hernanponcedeleon hernanponcedeleon changed the title 717 Fix default configuration for litmus tests Aug 26, 2024
@hernanponcedeleon hernanponcedeleon merged commit 48d880d into development Aug 26, 2024
1 check passed
hernanponcedeleon added a commit that referenced this pull request Aug 28, 2024
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.

4 participants