-
Notifications
You must be signed in to change notification settings - Fork 1k
[naga const-eval] LiteralVector
and some demo builtins
#6213
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
base: trunk
Are you sure you want to change the base?
Conversation
LiteralVector
and some demoLiteralVector
and some demo builtins
Any thought @teoxoy @ErichDonGubler ? |
Hoo boy. This looks really involved, and I definitely haven't yet taken the time to dig into everything. I'm not sure if I'll be able to prioritize this within the week, but I will say this: Something that allows more |
I do not have CTS results yet, as I didn't handle all necessary types in dot,cross yet.
Actually this should be correct per spec, just not optimal performance wise (due to redundant checking). |
wgpu/naga/src/front/wgsl/lower/mod.rs Line 1369 in 0e352f5
|
results from [servo's CTS run:](https://github.com/sagudev/servo/actions/runs/10859244967)
|
|
gentle ping @ErichDonGubler SIDE-NOTE: One thing that is annoying is that we do validation in validator and here in const eval (although most error cases should be unreachable here due to validator). |
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.
Hey @sagudev! I actually was assuming that you'd bring this PR out of draft before I should review it. The new PASS
es from your Servo test run are exciting. I didn't notice any regressions, but wanted to confirm: are there any? If there are no regressions, I don't see a reason why we shouldn't review this immediately, and iterate on this code further for readability, etc.
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.
thought: The large match
es generated for LiteralVector
seem like they could be better served with a macro_rules!
macro to generate code; I think it'd be easier to read (i.e., see the patterns, since they're explicit) and modify code down the line.
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.
Macros are hard. Here is a prototype I wrote during weekend: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=846b65db0f424adf84653355c32815a4 (there are many macros limitations that were limiting me such as that macros must only return expr not patterns and repetition nesting rules).
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.
TBH I am not really happy how macro turned out in 21acc9d
What do you think @ErichDonGubler?
I first wanted to get green light for idea/concept. I keep my PRs as draft until they are actually ready to land (at least from my side), to prevent accidental lands.
No there aren't any (at least in old run). |
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
rebased! |
Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
Connections
Implements some #4507
Description
Existing macros work for component-wise cases where return type is equal to input type. But it's hard to generalize macros further. So I present
LiteralVector
:that abstracts away at least vector obtaining logic. This makes somehow easier to implement some builtins like
dot
,cross
,any
,all
, ... although with still way to much repetition (maybe macro could fix that, although I am more inclined to use num create for number traits).Testing
TODO
Checklist
cargo fmt
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
--target wasm32-unknown-emscripten
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.