-
Notifications
You must be signed in to change notification settings - Fork 353
Add syntax sugar for dereferencing composites #4311
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
Conversation
|
Previews, as seen when this build job started (c0a650f): |
dneto0
left a comment
There was a problem hiding this 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.
mehmetoguzderin
left a comment
There was a problem hiding this 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)
790a110 to
c38a6e5
Compare
dneto0
left a comment
There was a problem hiding this 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.
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`
c38a6e5 to
9eebedf
Compare
jimblandy
left a comment
There was a problem hiding this 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.
|
@dneto0 I think it would make sense to extend this to swizzles as well. I think it would be surprising to people if |
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
|
I've added the multi-component selection changes and fixed a couple spots in valid and invalid memory references. PTAL. |
dneto0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you!
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]>
Fixes #4114
pointer-composite-access