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.

Sign up for GitHub

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

Jump to bottom

[WGSL] Variables initializer shouldn't be a reference #14584

Merged
merged 1 commit into from
Jun 5, 2023
Merged

[WGSL] Variables initializer shouldn't be a reference #14584

merged 1 commit into from
Jun 5, 2023

Conversation

tadeuzagallo
Copy link
Member

@tadeuzagallo tadeuzagallo commented Jun 1, 2023 •
edited by webkit-early-warning-system

e90fdfd

[WGSL] Variables initializer shouldn't be a reference
https://bugs.webkit.org/show_bug.cgi?id=257601
rdar://110105334

Reviewed by Mike Wyrzykowski.

When initializing a variable, the value used to initialize it should be unpacked
if it's a reference. E.g.:

var a = 0;
var b = a;
b = 1;

The above program should not change the value of `a`, and references to `b` should
have type `ref<i32>`, not `ref<ref<i32>>`. In order to fix that, when we infer the
type of a variable initializer (which doesn't have an explicit type annotation), if
the is a reference (as it would be for `a` in the example above, which would have type
`ref<i32>`), we should use the referenced type instead (i.e. `i32` in this example)

* Source/WebGPU/WGSL/TypeCheck.cpp:
(WGSL::TypeChecker::visitVariable):

Canonical link: https://commits.webkit.org/264860@main

b3d8654

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2   🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt   🧪 gtk-wk2
✅ 🧪 api-ios   🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv   🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@tadeuzagallo tadeuzagallo self-assigned this Jun 1, 2023
@tadeuzagallo tadeuzagallo added the WebGPU For bugs in WebGPU label Jun 1, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 3e81c6a)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
❌ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@tadeuzagallo tadeuzagallo added the merge-queue Applied to send a pull request to merge-queue label Jun 5, 2023
@webkit-commit-queue
Copy link
Collaborator

No reviewer information in commit message, blocking PR #14584

@webkit-commit-queue webkit-commit-queue added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Jun 5, 2023
@tadeuzagallo tadeuzagallo removed the merging-blocked Applied to prevent a change from being merged label Jun 5, 2023
@tadeuzagallo tadeuzagallo force-pushed the eng/WGSL-Variables-initializer-shouldnt-be-a-reference branch from 3e81c6a to b3d8654 Compare June 5, 2023 08:08
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash b3d8654)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2   🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt   🧪 gtk-wk2
✅ 🧪 api-ios   🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv   🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@tadeuzagallo tadeuzagallo added the merge-queue Applied to send a pull request to merge-queue label Jun 5, 2023
@tadeuzagallo
[WGSL] Variables initializer shouldn't be a reference
e90fdfd 
https://bugs.webkit.org/show_bug.cgi?id=257601
rdar://110105334

Reviewed by Mike Wyrzykowski.

When initializing a variable, the value used to initialize it should be unpacked
if it's a reference. E.g.:

var a = 0;
var b = a;
b = 1;

The above program should not change the value of `a`, and references to `b` should
have type `ref<i32>`, not `ref<ref<i32>>`. In order to fix that, when we infer the
type of a variable initializer (which doesn't have an explicit type annotation), if
the is a reference (as it would be for `a` in the example above, which would have type
`ref<i32>`), we should use the referenced type instead (i.e. `i32` in this example)

* Source/WebGPU/WGSL/TypeCheck.cpp:
(WGSL::TypeChecker::visitVariable):

Canonical link: https://commits.webkit.org/264860@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/WGSL-Variables-initializer-shouldnt-be-a-reference branch from b3d8654 to e90fdfd Compare June 5, 2023 08:53
@webkit-commit-queue
Copy link
Collaborator

Committed 264860@main (e90fdfd): https://commits.webkit.org/264860@main

Reviewed commits have been landed. Closing PR #14584 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit e90fdfd into WebKit:main Jun 5, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mwyrzykowski mwyrzykowski mwyrzykowski approved these changes

@litherum litherum Awaiting requested review from litherum

@djg djg Awaiting requested review from djg djg is a code owner

Assignees

@tadeuzagallo tadeuzagallo

Labels
WebGPU For bugs in WebGPU
Projects
None yet
Milestone
No milestone
4 participants
@tadeuzagallo @webkit-early-warning-system @webkit-commit-queue @mwyrzykowski