Skip to content

Conversation

@ben-clayton
Copy link
Contributor

And annotate examples that are expected to not compile with expect-error

Copy link
Contributor

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Just one note, otherwise looks great!
We'll need to make it so CI checks these examples to load as correct WGSL modules.

@ben-clayton ben-clayton force-pushed the fix-examples branch 2 times, most recently from 1786298 to 3ab5a4b Compare January 6, 2021 18:58
And annotate examples that are expected to not compile with `expect-error`
@ben-clayton
Copy link
Contributor Author

We'll need to make it so CI checks these examples to load as correct WGSL modules.

There's a bit of a chicken-and-egg problem with doing this. Whatever the reference compiler is, it'll need to be updated before the spec in order for the tests to pass.
Maybe once everything has stabilised a bit more we can enable this.

FWIW, the tool I'm writing for this can easily be adapted to check against your compiler. If you're interested in this, please shout.


<div class='example wgsl global-scope' heading='Entry Point'>
<xmp>
[[builtin(position)]] var<out> gl_Position : vec4<f32>;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably stop calling these gl_xxx :)

@ben-clayton ben-clayton merged commit 32862dc into gpuweb:main Jan 6, 2021
@ben-clayton ben-clayton deleted the fix-examples branch January 6, 2021 19:41
@ben-clayton
Copy link
Contributor Author

FWIW, the tool I'm writing for this can easily be adapted to check against your compiler. If you're interested in this, please shout.

@kvark - if you are interested, the tool has been committed and lives here
There's a couple of things we might need to change to make it work with your compiler:

  1. The command line arguments to the compiler might need changing
  2. There's currently a fudgy workaround to inject an entrypoint if the example doesn't already contain one (few do). I'd suspect your compiler would emit a different error in this case, and so we'd need to pattern match for both (or add even more markup to the spec).

Anyway, give me a shout if you want any changes. I'm happy to make this more generic and generally useful.

@kvark
Copy link
Contributor

kvark commented Jan 7, 2021

@ben-clayton nice! Can this tool live separately from Tint and be prepared to work with an abstract implementation instead of Tint specifically? I'd love to add support for naga to it.

@ben-clayton
Copy link
Contributor Author

Can this tool live separately from Tint

Sure. Any suggestions of a better home? Given that it's a just single file, what about this repo?

and be prepared to work with an abstract implementation instead of Tint specifically?

The two things listed above are the only tint specific things - it simply shells the executable you specify with temporary wgsl files extracted from the spec. If you can let me know how you invoke the naga compiler, and what sort of error you get when attempting to compile a shader with no entry point, I can easily adjust it to work with both compilers.

@kvark
Copy link
Contributor

kvark commented Jan 8, 2021

Imagine a project that is a WebGPU-based engine. They may want to accept WGSL and provide examples of it in their documentation. They may want to use your script to check them. We wouldn't want to require them to depend on the whole WebGPU repo, unless you expect them to just copy the script over and re-vendor it in their own repo?

If you can let me know how you invoke the naga compiler, and what sort of error you get when attempting to compile a shader with no entry point, I can easily adjust it to work with both compilers.

There is nothing in WGSL that requires a shader module to have entry points.
Generally, Naga is not a binary, it's a library, so it can't be invoked. We do have an example binary though, called convert. It can be built with options to only do WGSL validation:

cd naga
cargo build --example convert --features wgsl-in
target/debug/examples/convert <file.wgsl>

@ben-clayton
Copy link
Contributor Author

Imagine a project that is a WebGPU-based engine. They may want to accept WGSL and provide examples of it in their documentation. They may want to use your script to check them. We wouldn't want to require them to depend on the whole WebGPU repo, unless you expect them to just copy the script over and re-vendor it in their own repo?

I think this is well beyond the scope of the tool I've written. The bulk of the code is to fetch the spec, parse the HTML as emitted by Bikeshed along with the bespoke class tags I've invented. If you want a generic tool, you'd likely gut most of that, leaving nothing more than something that generates a temporary file, invokes an executable N times, check result script. That can be accomplished in less than 100 lines of most scripting languages. I think attempting to support this use case will end up with far more code to support the use case than just writing the tool from scratch.

There is nothing in WGSL that requires a shader module to have entry points

The validation rules appear to disagree - see: v-0003

That said, if naga doesn't have any issues with compiling a shader with no entry point, then the workaround will not need to be used, so no changes will likely to be needed here.

Generally, Naga is not a binary, it's a library, so it can't be invoked. We do have an example binary though, called convert.

Same with tint. We build an 'example' binary called tint though :)

target/debug/examples/convert <file.wgsl>

Awesome, exactly the same args as the tint exe. No changes required here.

Sounds like the tool might work for you without any modifications.

@kvark
Copy link
Contributor

kvark commented Jan 8, 2021

I don't understand why WGSL needs this validation rule, honestly. It's not helping anybody.

ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022
* Move test device selection to an optional before clause

The beforeAllSubcases function is an optional function which
will be run once for each testcase, but not for subcases inside that
testcase. This allows some common code to be performed ahead of
time and shared between subcases. This is immediately useful so
that device selection out of the device pool can be performed once
and then shared for all of the subcases. Subcases should not use different
devices and will be refactored to select the device in the
beforeAllSubcases function, without relying on subparams.

The before function operates on a SubcaseBatchState which each
Fixture class may override. This state object is shared between all
fixtures created for subcases within a single testcase.

The test device is stored in this shared state so that now, reserving a
device from the device pool occurs once for all subcases, it is shared
between the subcases, and released at the end of the testcase.

This means that if there are unexpected validation, OOM, or device lost
errors, they will only be reported at testcase granularity, not subcase
granularity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants