Bools are IO-shareable, but only for builtins by dneto0 · Pull Request #1246 · gpuweb/gpuweb

dneto0 · 2020-11-20T22:09:06Z

dj2 · 2020-11-21T00:09:44Z

 * Values written as output for downstream processing in the pipeline, or to an output attachment.

+Note: Only built-in pipeline inputs may have a boolean type.
+A user data attribute input or output must not be of [=bool=] type or contain a [=bool=] type.


This reads strangely. Maybe A user input or output data attribure ..?

dj2 · 2020-11-21T00:10:28Z

-Note: An [=IO-shareable=] type would also be host-shareable if it and its subtypes have
-the approporate [=stride=] and [=offset=] attributes.
+Note: An [=IO-shareable=] type *T* would also be host-shareable if *T* and its subtypes have
+appropriate [=stride=] and [=offset=] attributes, and if *T* is not [=bool=] and did


nit: did -> does ?

kvark

Why can't the user I/O data have bools? Do we have the relevant investigation somewhere?

kainino0x · 2020-11-23T19:26:41Z

OpTypeBool

Declare the Boolean type. Values of this type can only be either true or false. There is no physical size or bit pattern defined for these values. If they are stored (in conjunction with OpVariable), they must only be used with logical addressing operations, not physical, and only with non-externally visible shader Storage Classes: Workgroup, CrossWorkgroup, Private, Function, Input, and Output.

kainino0x · 2020-11-23T19:29:57Z

And you probably wouldn't want to have WGSL give them a bit pattern (i.e. define bools as translating to OpTypeInt) unless you had 1-byte storage (not possible in core), otherwise they would waste bandwidth (at 4 bytes per bool).

kvark · 2020-11-24T15:45:19Z

@kainino0x what you posted actually agrees with my question and contradicts @dneto0 's PR: it says Input and Output are valid storage classes for bools. Where does it say that Input and Output bools are only allowed for builtins?

grorg · 2020-12-01T19:11:21Z

Discussed at the 2020-12-01 meeting.

Resolution: Accept with the update. @kvark will file a post-MVP issue on relaxing the rule.

Fixes gpuweb#1244

dneto0 · 2020-12-08T19:48:04Z

I think I've addressed your feedback. PTAL @dj2

dneto0 · 2021-01-15T23:28:32Z

@kvark asked

Where does it say that Input and Output bools are only allowed for builtins?

OpTypeBool can't be on the pipeline input or output boundary because OpTypeBool does not have a concrete bit representation.

There's this note in the SPIR-V spec:

Khronos Issue #363: OpTypeBool can be used in the Input and Output storage classes, but the client APIs still only allow built-in Boolean variables (e.g. FrontFacing), not user variables.

(I'm looking for a more definitive statement in Vulkan...)

dneto0 · 2021-01-15T23:32:11Z

Ah, here it is. From 15.1.2 User-defined Variable Interface

The non-built-in variables listed by OpEntryPoint with the Input or Output storage class form the user-defined variable interface. These must have SPIR-V numerical types or, recursively, composite types of such types.

That excludes bool.

dneto0 · 2021-01-19T20:05:58Z

Discussed in 2021-01-19 meeting

@kvark suggested allowing bool on input and output, and polyfilling for Vulkan, with 0 interpreted as false, non-zero as true.

kdashg · 2021-01-26T20:50:27Z

WGSL meeting minutes 2021-01-26

JG: Is a PR
DN: Requires polyfill on vulkan. Readings not so bad, reading would have to detect you’re writing and translate the output
MM: Also not hard, yea?
DN: Need to know the right var. If it is a pointer to a helper then we need to know.
DM: From SPV perspective, if you have a pointer to the output storage class, then you know it can’t be bool.
DN : Currently yes.
DM: Then not too bad, can forbid for now and make it available later.
MM: For now, with logical mode, we know what all pointers point too.
JG: Seems implementable.
DN: In a helper function ..
MM: Would have to specialize the helper
DN: Everything we’ve done has avoided inlining as the solution to the problem.
JG: Propose we try to do it, and if it doesn’t work we remove it.
MM: Another solution, we take advantage of the representation of pointers being unspecified so you accompany if this pointer points to the thing with the pointer.
JG: Easier on metal then spirv.
MM: In metal you can literally pack, in SPIR-V would need 2 values.
DN: Would handle this before SPIR_v code gen. Would have to think through the implementation.
JG: Rough consensus.
MM: Which bool outputs?
DN: We have bool input for builtins.
MM: Right, so don’t have to solve just yet.
JG: True, unless we allow assigning front facing.
MM: Don’t think any api allows that.
DN: The diff of opinion is for user variables, not the builtins.
MM: Don’t think we should allow for user variables. Just builtins.
DN: That’s what 1246 is trying to change, trying to allow for user variables to be bool
MM: Doh.
JG: Can we re-title this. Not really what the title says
DN: Started with bools not io sharabale, then allowed front-facing to be bool. Then tough we should extend to everyone.
MM: So if you wanted to vtx shader output as a bool, you’d just write a 1 or 0 int and the rasterizer would do the thing and the frag shader would read a 1 or 0
DM: Interpolation question is separate, relates to other things then bool
JG: Think you should use interpolatable things instead of doing with bool
DM: Can’t require it on integer either
MM: There is flat shading, for flat shading this isn’t strange.
DM: Are we planning to allow structures to be varying between shader stages
MM: I’d say no
DN: I’d say yes. You scalaraize it and everything gets a location
MM: Would make a different perf argument. Usually performance is sensitive to the number of these streams. Should be hard to accidentally output 700 of them
DN: Isn’t the issue not how it’s packaged but the number of words going across?
MM: Think it should be more typing to output many varying rather than one varying. Think it would be too easy to have a struct with a billion fields that they output. Easy to write that program, less performant for the machine.
DN: So, the conclusion was we shouldn’t allow structs to go across?
MM: Yes
DN: Thought the question is if we would disallow a mis-match of the data
JG: Thought it was the struct thing
DM: People want to store bools in structs and want to pass between shader stages.
JG: Would be convenient.
DN: Related question. With polyfil, does bool -> int match?
MM: Yuck
JG: Everything has to match
MM: Already agreed everything has to match. Structs are defined by their name, not content. Two structs with the same fields and different names, do they match?
DN: Extensional vs [...missing]
JG: For the purposes of this PR, sounds like rough agreement to take at least this, but expand later for bool variables. This is needed now for front_facing.
MM: Would be a soft thumbs down on allowing bool outputs from vertex shaders.
JG: Will file an issue on that
DN: So DM Will you file a post MVP enhancement?
DM: Sure.

dneto0 · 2021-01-26T21:31:50Z

Ok, so there is agreement on the content. Now this PR just needs editorial review.

This CL adds unimplemented stub tests for the `transpose` builtin. Issue: gpuweb#1246

dneto0 added wgsl WebGPU Shading Language Issues for wgsl meeting labels Nov 20, 2020

dneto0 added this to the MVP milestone Nov 20, 2020

dneto0 requested a review from kvark November 20, 2020 22:09

dj2 reviewed Nov 21, 2020

View reviewed changes

kvark reviewed Nov 23, 2020

View reviewed changes

kvark mentioned this pull request Dec 1, 2020

[post-MVP] Support booleans as input/output between stages #1268

Open

dneto0 added 2 commits December 8, 2020 14:22

Bools are IO-shareable, but only for builtins

31fb9fb

Fixes gpuweb#1244

Apply review feedback

d157e69

dneto0 force-pushed the bool-io-shareable branch from d0c47c5 to d157e69 Compare December 8, 2020 19:36

dneto0 added wgsl resolved Resolved - waiting for a change to the WGSL specification and removed for wgsl meeting labels Dec 15, 2020

dneto0 added the for wgsl meeting label Jan 15, 2021

kdashg removed the for wgsl meeting label Jan 19, 2021

kvark mentioned this pull request Jan 26, 2021

Support boolean varyings #1379

Closed

dj2 approved these changes Jan 26, 2021

View reviewed changes

dneto0 merged commit 2386650 into gpuweb:main Jan 27, 2021

ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022

wgsl: Stub tests for the transpose builtin. (gpuweb#1354)

9646ebe

This CL adds unimplemented stub tests for the `transpose` builtin. Issue: gpuweb#1246

Conversation

dneto0 commented Nov 20, 2020

Uh oh!

dj2 Nov 21, 2020

Choose a reason for hiding this comment

Uh oh!

dj2 Nov 21, 2020

Choose a reason for hiding this comment

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

kainino0x commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kainino0x commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark commented Nov 24, 2020

Uh oh!

grorg commented Dec 1, 2020

Uh oh!

dneto0 commented Dec 8, 2020

Uh oh!

dneto0 commented Jan 15, 2021

Uh oh!

dneto0 commented Jan 15, 2021

Uh oh!

dneto0 commented Jan 19, 2021

Uh oh!

kdashg commented Jan 26, 2021

Uh oh!

dneto0 commented Jan 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kainino0x commented Nov 23, 2020 •

edited

Loading

kainino0x commented Nov 23, 2020 •

edited

Loading