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 the compute limits, adds maxComputeWorkgroupSize #1898

Merged
merged 7 commits into from
Jul 13, 2021

Conversation

Kangz
Copy link
Contributor

@Kangz Kangz commented Jul 1, 2021

Validation was incorrectly checking values were above limits instead of
below limits.

maxComputeWorkgroupSize is necessary because the 3rd component of
workgroup_size is almost always 64 in Vulkan devices (on mobile and
desktop).


Preview | Diff

Validation was incorrectly checking values were above limits instead of
below limits.

maxComputeWorkgroupSize is necessary because the 3rd component of
workgroup_size is almost always 64 in Vulkan devices (on mobile and
desktop).
@Kangz Kangz requested review from kvark and kainino0x July 1, 2021 14:47
@github-actions
Copy link
Contributor

github-actions bot commented Jul 1, 2021

Previews, as seen when this build job started (7753ba0):
WebGPU | IDL
WGSL
Explainer

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
Kangz and others added 3 commits July 2, 2021 09:37
Co-authored-by: Kai Ninomiya <kainino1@gmail.com>
Co-authored-by: Kai Ninomiya <kainino1@gmail.com>
Co-authored-by: Kai Ninomiya <kainino1@gmail.com>
@Kangz
Copy link
Contributor Author

Kangz commented Jul 2, 2021

Thanks for the suggestions, applied all of them.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2021

Previews, as seen when this build job started (e53244c):
WebGPU | IDL
WGSL
Explainer

@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2021

Previews, as seen when this build job started (51d1835):
WebGPU | IDL
WGSL
Explainer

@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2021

Previews, as seen when this build job started (52aa85e):
WebGPU | IDL
WGSL
Explainer

@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2021

Previews, as seen when this build job started (52aa85e):
WebGPU | IDL
WGSL
Explainer

Copy link
Contributor

@alan-baker alan-baker left a comment

Choose a reason for hiding this comment

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

Out of curiosity, why are the base limits for workgroup size and invocations higher than Vulkan? Vulkan only provides (128,128,64) for size and 128 for invocations by default.

spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
Kangz and others added 2 commits July 2, 2021 17:01
Co-authored-by: alan-baker <alanbaker@google.com>
Co-authored-by: alan-baker <alanbaker@google.com>
@Kangz
Copy link
Contributor Author

Kangz commented Jul 2, 2021

It is because looking at vulkan.gpuinfo.org the only devices that don't support (256, 256, 64) are Swiftshader (which I just fixed in this CL) and a few ARM devices that correspond to very old Mali.

We try to support a wide variety of Android Vulkan devices but are ok removing some percentage of them to raise WebGPU's core feature level. The others can be supported through lower-feature levels on GLES in the future.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2021

Previews, as seen when this build job started (5eb5a41):
WebGPU | IDL
WGSL
Explainer

@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2021

Previews, as seen when this build job started (8829596):
WebGPU | IDL
WGSL
Explainer

@kainino0x
Copy link
Contributor

I'm curious whether there are any devices where maxComputeWorkgroupSize.x or maxComputeWorkgroupSize.y is less than maxComputeWorkgroupInvocations (or if there are, whether it matters). It seems unlikely, at least for x. Maybe we only need maxComputeWorkgroupInvocations and maxComputeWorkgroupSizeZ?

We should also add some kind of guarantee that the adapter limits make sense maxX <= maxXYZ && maxY <= maxXYZ && maxZ <= maxXYZ and probably maxXYZ <= maxX * maxY * maxZ though it matters less. (I probably wouldn't disallow setting weird limits in requestDevice, but we could.)

For example, on this random phone (first one I looked at), the max is [1024,1024,64] but total 512 (I guess you can use [1024,1,0], but [1024,1,1] is invalid??). If an app saw these limits it would be rather likely to try to use [1024,1,1] and fail unexpectedly.

Aside: now that I've been looking at it repeatedly, "workgroup invocations" confused me, it sounds like "number of workgroup invocations" not "number of invocations in a workgroup". Should we rename to, like, maxComputeWorkgroupVolume?

@Kangz
Copy link
Contributor Author

Kangz commented Jul 5, 2021

+1 on using some computeWorkgroupVolume terminology and having some guarantees on the relations between some of the limits in the spec. However I'd rather not merge maxComputeWorkgroupSize.x and maxComputeWorkgroupSize.y together even if we're not able to find devices with different limits there: such devices might exist in the future, but also I think it is a net win for developer understanding to have the limit exposed as a "vector".

Copy link
Contributor

@kvark kvark left a comment

Choose a reason for hiding this comment

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

The observation that Kai makes is quite interesting. We shouldn't define limits "just in case" the future hardware comes and needs them. We should be ready to have Limits V2 at some point, anyway. Hopefully, not soon :)

spec/index.bs Outdated Show resolved Hide resolved
@@ -1282,6 +1288,7 @@ interface GPUSupportedLimits {
readonly attribute unsigned long maxInterStageShaderComponents;
readonly attribute unsigned long maxComputeWorkgroupStorageSize;
readonly attribute unsigned long maxComputeWorkgroupInvocations;
readonly attribute GPUExtent3D maxComputeWorkgroupSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the GPUExtent3D is a bit strange here. It's made for texture sizes specifically, hence depthOrArrayLayers field, but the compute workgroup grid has no direct relation to the texture sizes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is readonly and described as [256, 256, 64] above. The intent is that it is always the array version. I agree that the parallels with texture size are weird because they should be non-existent, but we're really talking about a volume here so it makes sense to use Extent3D imho.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the intent is to use the array, any reason to not put sequence<GPUIntegerCoordinate> in here?
I don't know how users are expected to deal with GPUExtent3D coming from us. It's one thing to pass in either a list or a structure. It's another thing to not know what to do with this type when it comes to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is because WebIDL doesn't have [GPUIntegerCoordinate; 3] to show the sequence is statically sized, while GPUExtent3D is clearly sized. But happy to change, this doesn't really matter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, no strong feelings here, we can follow-up

@github-actions
Copy link
Contributor

Previews, as seen when this build job started (2479ab4):
WebGPU | IDL
WGSL
Explainer

@kainino0x
Copy link
Contributor

+1 on using some computeWorkgroupVolume terminology

Editors came up with "maxComputeInvocationsPerWorkgroup"

All together that would be:

  • maxComputeWorkgroupStorageSize, maybe maxWorkgroupStorage
  • maxComputeInvocationsPerWorkgroup, maybe maxInvocationsPerWorkgroup
  • maxComputeWorkgroupSize, maybe maxComputeWorkgroupDimensions

@kvark kvark merged commit 8c00aa7 into gpuweb:main Jul 13, 2021
github-actions bot added a commit that referenced this pull request Jul 13, 2021
SHA: 8c00aa7
Reason: push, by @kvark

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 13, 2021
SHA: 8c00aa7
Reason: push, by @kvark

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Jul 13, 2021
SHA: 8c00aa7
Reason: push, by @kvark

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Kangz Kangz deleted the fix-limits branch July 15, 2021 12:13
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.

None yet

4 participants