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

Compat: Refactor memory module tests to use 4 storage buffers #3117

Merged
merged 2 commits into from
Nov 28, 2023

Conversation

greggman
Copy link
Contributor

So ... maybe this test was written against older WGSL but it's not clear to me why the code was

struct IndexMemory {
  value: array<u32>,
};

@group(0) @binding(2) var<storage, read_write> foo: IndexMemory;

Instead of just

@group(0) @binding(2) var<storage, read_write> foo: array<u32>;

But, because it was already that way, the smallest change seemed to be to leave it that way. I can go change them all to get rid of the struct and just use arrays directly. Access would switch from combo.thing.value[ndx] to combo.thing[ndx].

Similarly, it's unclear why barrier is defined as an array and not just a single value. I could change that as well assuming there isn't some reason it's an array.

Also, compat has a maxWorkgroupSizeX of 128. I switched the code to use 128 in compat and didn't change any other code. It passes. I'd have expected to have to change something else to say "only check 128 results" or something to that effect. That makes me concerned the test is not actually testing anything. I didn't dig though.

Issue: #3049


Requirements for PR author:

  • All missing test coverage is tracked with "TODO" or .unimplemented().
  • New helpers are /** documented */ and new helper files are found in helper_index.txt.
  • Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

Requirements for reviewer sign-off:

  • Tests are properly located in the test tree.
  • Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
  • Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
  • Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

@greggman greggman requested a review from dneto0 October 31, 2023 08:17
@github-actions
Copy link

Previews, as seen when this build job started (8ed631b):
Run tests | View tsdoc

@greggman
Copy link
Contributor Author

greggman commented Nov 4, 2023

ping

Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on.
Thank you for your patience as well.

I'm requesting two changes:

  • use the same tweaked workgroup throughout
  • avoid false sharing between the sub-buffers.

@dneto0
Copy link
Contributor

dneto0 commented Nov 24, 2023

So ... maybe this test was written against older WGSL but it's not clear to me why the code was

It was written quite some time ago. Originated here: https://gpuharbor.ucsc.edu/webgpu-mem-testing/

I'm not particularly bothered, but thank you for avoiding style-based rewrites in this PR.

@dneto0
Copy link
Contributor

dneto0 commented Nov 24, 2023

So ... maybe this test was written against older WGSL but it's not clear to me why the code was

struct IndexMemory {
  value: array<u32>,
};

@group(0) @binding(2) var<storage, read_write> foo: IndexMemory;

Instead of just

Also, compat has a maxWorkgroupSizeX of 128. I switched the code to use 128 in compat and didn't change any other code. It passes. I'd have expected to have to change something else to say "only check 128 results" or something to that effect. That makes me concerned the test is not actually testing anything. I didn't dig though.

For the long answer, read the "MC Mutants" paper: https://dl.acm.org/doi/10.1145/3575693.3575750

The short answer is @reeselevine did experiments to heuristically search for ways to evoke weak memory behaviours via various techniques (read the paper). The 256 is basicaly arbitrary. Changing it to 128 is still valid bit pushes it into an unexamined regime. My intuition is the test is probably as effective as before, or close to it. IMHO it's good to keep it 256 for baseline WebGPU and limit it to 128 for the compat case. It's fine, no sweat.

Does it do anything? Yes.
For example go to https://gpuharbor.ucsc.edu/webgpu-mem-testing/tests/message-passing/ and hit the "Start Test" button at the bottom. You are likely to see weak behaviours exhibited (blue and red bars). That's success. It's doing something.

@greggman greggman force-pushed the compat-memory-model branch from 8ed631b to e150fde Compare November 28, 2023 00:57
@greggman
Copy link
Contributor Author

Thanks David! I think I addressed the issues you brought up. PTAL. Had to force push because I needed to deal with Kai's changes here #3114

Copy link
Contributor

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

LGTM.
Your choice whether to use align instead of Math.max. align would be better.

thanks

@greggman greggman enabled auto-merge (squash) November 28, 2023 17:02
@greggman greggman merged commit f2546cb into gpuweb:main Nov 28, 2023
1 check passed
@greggman greggman deleted the compat-memory-model branch November 28, 2023 17:24
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.

2 participants