Skip to content

Conversation

@alan-baker
Copy link
Contributor

Fixes #4114

  • Add equivalent syntax for accessing components of a composite via pointer as when accessing via a reference
  • Introduces a new language extension pointer-composite-access

@alan-baker alan-baker added wgsl WebGPU Shading Language Issues language extension labels Oct 2, 2023
@alan-baker alan-baker added this to the Milestone 1 milestone Oct 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023

Previews, as seen when this build job started (c0a650f):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

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.

Suggestion for the wording describing the feature.

Copy link
Member

@mehmetoguzderin mehmetoguzderin left a comment

Choose a reason for hiding this comment

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

LGTM (while agreeing with the suggestion about dereference)

@alan-baker alan-baker force-pushed the pointer-composite-access branch from 790a110 to c38a6e5 Compare October 9, 2023 23:38
@alan-baker alan-baker requested a review from dneto0 October 9, 2023 23:38
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. I would like to have @jimblandy weigh in on whether the feature description is descriptive enough of what's being added.

@dneto0 dneto0 requested a review from jimblandy October 10, 2023 19:15
Fixes gpuweb#4114

* Add equivalent syntax for accessing components of a composite via
  pointer as when accessing via a reference
* Introduces a new language extension `pointer-composite-access`
@alan-baker alan-baker force-pushed the pointer-composite-access branch from c38a6e5 to 9eebedf Compare November 21, 2023 20:27
Copy link
Contributor

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

It seems like this falls out very nicely.

You may want to also update the language in "Valid and Invalid Memory References" as well, since named component expressions and indexing operations can now be applied to pointers.

Perhaps "Use Cases for References and Pointers" should also be updated - but it seems like that text is concerned with drawing distinctions. It's not clear anything needs to change here, although it's definitely in the neighborhood.

@jimblandy
Copy link
Contributor

jimblandy commented Nov 23, 2023

@dneto0 I think it would make sense to extend this to swizzles as well. I think it would be surprising to people if p.x worked but not p.xy. The "Vector Multiple Component Selection" rules would not produce a reference, but they would implicitly dereference a pointer base. (To be clear, I still approve this PR.)

@dneto0
Copy link
Contributor

dneto0 commented Nov 23, 2023

@dneto0 I think it would make sense to extend this to swizzles as well. I think it would be surprising to people if p.x worked but not p.xy. The "Vector Multiple Component Selection" rules would not produce a reference, but they would implicitly dereference a pointer base. (To be clear, I still approve this PR.)

That's a really good point and a nice catch. I agree that should be included.

Seems this upgrade should be in the "land it now and notify the committee" .

* Allow pointers as the base for multi-swizzle specifiers
  * Result is a value (not a reference)
  * The pointer is converted to a reference and then the load rule is
    invoked
* Clarify some language about references and pointers
@alan-baker alan-baker requested a review from dneto0 November 23, 2023 15:35
@alan-baker
Copy link
Contributor Author

I've added the multi-component selection changes and fixed a couple spots in valid and invalid memory references. PTAL.

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. Thank you!

@alan-baker alan-baker merged commit 2b6430d into gpuweb:main Nov 24, 2023
@alan-baker alan-baker deleted the pointer-composite-access branch November 24, 2023 18:54
copybara-service bot pushed a commit to google/dawn that referenced this pull request Jan 11, 2024
Adds support to Tint for syntax sugar for dereferencing pointers for
member or index access as per:
gpuweb/gpuweb#4311

- Resolver: when the lhs of a accessor expression is a pointer, it is
  now resolved to a sem::Reference.
- Added "pointer_composite_access" feature as experimental, hooked up
  validation in Resolver, and added tests.
- Added resolver tests for the new syntax to resolver/ptr_ref_test.cc.
- Fixed multiple transforms to deal with the fact that the lhs of
  accessor expressions can now be pointers, including: Robustness,
  Renamer, Std140, and SimplifyPointers.
  - In transforms that rely on other transforms, such as
    SimplifyPointers, to remove/inline pointers, I added asserts that
    the type is not a pointer.
  - Added unit tests for transforms that use pointer-dot/index for
    accessor expressions.
- Fixed uniformity analysis code so that ProcessLValueExpression
  correctly deals with accessor expressions where the object is a
  pointer, in the same way we do for UnaryOp::kIndirection, including
  partial pointer checks. Added many tests for these new cases.
- Fixed ProgramToIR so that EmitAccess handles the new syntax. Added
  multiple tests.
- Added end2end tests under test/tint/ptr_sugar

For Googlers, see my work log at
go/add-syntax-sugar-for-dereferencing-composites for more details.

Bug: tint:2113
Change-Id: I7a0093f52ca2237be598e44245b45049f21d056c
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/164900
Kokoro: Kokoro <[email protected]>
Reviewed-by: Corentin Wallez <[email protected]>
Reviewed-by: James Price <[email protected]>
Reviewed-by: Ben Clayton <[email protected]>
Commit-Queue: Antonio Maiorano <[email protected]>
amaiorano added a commit to amaiorano/cts that referenced this pull request Jan 22, 2024
amaiorano added a commit to amaiorano/cts that referenced this pull request Jan 23, 2024
amaiorano added a commit to amaiorano/cts that referenced this pull request Jan 24, 2024
amaiorano added a commit to gpuweb/cts that referenced this pull request Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

language extension wgsl WebGPU Shading Language Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add arrow operator as sugar for (*var).

4 participants