SymbolTable::New() used to build and return a symbol without a registered name. When you asked for the name of the symbol it would return tint_symbol_N, where N is the numerical identifier for the symbol.
This approach was a major tripping hazzard for transforms that liked to fetch the source program name, and register it in the new program (in this situation, you should always use `CloneContext::Clone(Symbol)`).
Without special casing for unnamed symbols, you could end up promoting the unnamed symbol to a named symbol, and then colliding against a new unnamed symbol.
This is exactly what happened in tint:711.
Instead, with this change:
* The concept of unnamed symbols has been removed. All symbols now have a name.
* The signature of `SymbolTable::New()` has been changed to take a name parameter (which defaults to 'tint_symbol'). This can be used to create a new, unique named symbol (possibly with a suffix), which will not collide with any existing symbols. Note these symbols may still collide if `SymbolTable::Register()` is called with the same name. All Transforms that currently use `SymbolTable::Register()` will be fixed in another change.
* The CloneContext has been updated to use `SymbolTable::New()` instead of `Register()`. This means that any symbols defined before a clone will not collide.
* `CloneContext::CloneSymbols()` has been added which allows a transform to pre-clone all the symbols from the source program. This can be used to avoid the authored identifiers being suffixed with a number, in the case a transform calls New() before the symbol is cloned.
* `Symbol::to_str()` has been changed to return `$<id>` instead of `tint_symbol_N`. This is to avoid any confusion between the actual name and the symbol ID.
Bug: tint:711
Bug: tint:712
Change-Id: I526e4b49b7027545613859de487e6a275686107a
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47631
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
A simple regex based tool for fixing unit tests that fail due to unexpected output
Change-Id: I72c47abaff6d6f4ba8cd497240eadc171af0fec3
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47629
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Migrate this transform to using the transform::DataMap pattern for configuration.
Allows users to fully construct their transforms ahead of time, and pass in the configuration options for each run.
Bug: tint:389
Change-Id: Ie4a8bf80d7b09cfe7bdd4ef01287d994b6b9eb4f
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47626
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Auto-Submit: Ben Clayton <bclayton@google.com>
When finding the true-head, false-head, and potentially the
premerge-head blocks of an if-selection, there was an overly
aggressive check for the true-branch or false-branch landing
on a merge block interior to the if-selection. The check was
determining if the merge block actually corresponded to the selection
header in question. If not, then it was throwing an error.
The bug was that this check must be performed only if the
target in question is actually inside the selection body.
There are cases where the target could represent a structured
exit, e.g. to an enclosing loop's merge or continue, or
an enclosing switch construct's merge.
There is still a latent bug: if either the true branch
or false branch represent such a kLoopBreak, kLoopContinue, or
kSwitchBreak edge, then those are not properly generated.
That will be fixed in a followup CL.
Bug: tint:243, tint:494
Change-Id: I141cce07fa0a1dfe5fad20dd2989315e4cd7b688
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47482
Auto-Submit: David Neto <dneto@google.com>
Commit-Queue: Alan Baker <alanbaker@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Baker <alanbaker@google.com>
We define the canonical type as a type stripped of all aliases. For
example, Canonical(alias<alias<vec3<alias<f32>>>>) is vec3<f32>. This
change adds Resolver::Canonical(Type*) which caches and returns the
resulting canonical type. We use this throughout the Resolver instead of
UnwrapAliasIfNeeded(), and we store the result in semantic::Variable,
returned from it's Type() member function.
Also:
* Wrote unit tests for Resolver::Canonical()
* Added semantic::Variable::DeclaredType() as a convenience to
retrieve the AST variable's type.
* Updated post-resolve code (transforms) to make use of Type and
DeclaredType appropriately, removing unnecessary calls to
UnwrapAliasIfNeeded.
* Added IntrinsicTableTest.MatchWithNestedAliasUnwrapping to ensure we
don't need to pass canonical parameter types for instrinsic table
lookups.
* ProgramBuilder: added vecN and matMxN overloads that take a Type* arg
to create them with alias types.
Bug: tint:705
Change-Id: I58a3b62538356b8dad2b1161a19b38bcefdd5d62
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47360
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Otherwise warnings are not emitted unless there is also an error.
Change-Id: If417f75fbdd246b1f792a0f63b83274c8f2272e3
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47481
Auto-Submit: James Price <jrprice@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: James Price <jrprice@google.com>
Currently WGSL does not allow a mip-level to be specified for `sampleCompare`, and cs_5_1 does not allow SampleCmp to be used.
Bug: tint:684
Change-Id: I53f35205185a0a1ee0dd5224fa1d8cd0ffedbea1
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47429
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
The renamer logic in the HLSL writer was inconsistent, and actually
broke some valid shaders.
Change-Id: I8fbddc7e657f5509b18435fdf352a39d83c1b89c
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47224
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
It ignored entry point builtin parameters, meaning it was mostly useless.
Was only used by the FirstIndexOffset, which has been re-written.
Change-Id: I26abd69201576cf33b1a71e8e6a1b914871a2a74
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47223
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
No idea why the operator<<() functions have started moaning now.
Change-Id: I338b96c53888f4ddb8e42283a6dcda7708e567f0
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47431
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
* It didn't always produce valid WGSL (crbug.com/tint/687)
* It didn't handle builtins as entry point parameters
* It used hard-coded symbols that could collide
* It didn't use DataMap for input.
The new implementation addresses all of this.
Bug: tint:687
Change-Id: I447bec530b45414ebb8baeb4ee18261d73d1c0d2
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47222
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
Add semantic::Swizzle and semantic::StructMemberAccess, both deriving from MemberAccessorExpression
Add semantic::Function::Parameters() to list the semantic::Variable parameters for the function.
Change-Id: I8cc69f3738380c14f61d051ee2989be6194d148d
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47220
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Reviewed-by: James Price <jrprice@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Avoids the need for downstream users to manually run this transform.
Change-Id: I0c63e2fd8b6ad49b752ed1757370e386171481cb
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47440
Commit-Queue: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Auto-Submit: James Price <jrprice@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
With this change, the Validator is fully removed from Tint, including
from it's public API.
Fix: tint:642
Change-Id: Id4867cc3866bb2ea09eff499537d58b938d18f43
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47125
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Use the new CalculateArrayLength and DecomposeStorageAccess transforms to simplify storage buffer patterns before running the HLSL writer.
The HLSL writer now handles the InternalDecorations for the internal load, store, and buffer-length intrinsics.
GeneratorImpl::EmitStorageBufferAccessor() has now been entirely removed, as all this primitive load / store decomposition performed by DecomposeStorageAccess.
TODOs around runtime arrays have been removed, as this is now handled by CalculateArrayLength.
Bug: tint:185
Bug: tint:683
Change-Id: Ie25a527e7a22da52778c4477cfc22501de558a41
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46878
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Used to used to replace calls to arrayLength() with a value calculated from the size of the storage buffer.
Bug: tint:185
Change-Id: If7ddc8dad2ed3d20c1d76b5f48bfd2c7634f96e2
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46877
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Used to breakdown storage buffer reads and writes into primitive loads and stores.
Bug: tint:683
Change-Id: I92983deef7485bc1a6a5d9921ec82f8e19b09744
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46876
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Reviewed-by: James Price <jrprice@google.com>
Fix some SPIR-V tests that were wrongly expecting parameters to be
loaded from memory as a result of them not being const.
Change-Id: Ieab6f1edd4a4ba6efd226fe190e7a49b309048c5
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47281
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: James Price <jrprice@google.com>
Auto-Submit: James Price <jrprice@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
This is a commonly used pattern.
Change-Id: I698397c93c33db64c53cbe8662186e1976075b80
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47280
Auto-Submit: James Price <jrprice@google.com>
Commit-Queue: James Price <jrprice@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Also update comments and arch design to remove references to the
Validator.
Bug: tint:642
Change-Id: Ic0b4779ae4712a393ff209014daf25e23f32be6d
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47061
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Reserve enough memory for overloads, and avoid copies. Opportunistically
avoid copying Source instances in ProgramBuilder API.
Speeds up runs of test_unittests.exe by about 20% (33s to 26s on my
Windows desktop).
Change-Id: I6ba26043d7750eb1f123e29c53d253614974f960
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47060
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
HLSL has some pecular rules around structure constructors.
`S s = S(1,2,3)` is not valid, but `S s = {1,2,3}` is.
This matches the quirkiness with array initializers, so adjust the array
hoisting logic to also support structures.
Fixed: tint:702
Change-Id: Ifdcafd98292715ae2482f72ec06c87842176d270
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46875
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Reviewed-by: James Price <jrprice@google.com>
Now that all validation has been moved to the Resolver, we can delete
the Validator. This change removes everything except validator.h with
the public no-op API. We can remove this once we remove dependencies on
this public API in Dawn.
Bug: tint:642
Change-Id: I644cd900615509c9cdd57c375c6217a50126e36c
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47023
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
float3(0.0f) is not legal HLSL
Change-Id: I2fd20e9718f394c896c9515c4c9b88e490b2b758
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46874
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
HLSL's matrices are declared as <type>NxM, where N is the number of
rows and M is the number of columns. Despite HLSL's matrices being
column-major by default, the index operator and constructors actually
operate on row-vectors, where as WGSL operates on column vectors.
To simplify everything we use the transpose of the matrices.
This is the same approach taken by SPIRV-Cross.
Change-Id: I98860e11ff1a68132736980f694b2f68b633ef83
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46873
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
With type inference, declared_type_ may be null.
Check it is not null before calling type_name().
Change-Id: I3b7630286b75aa9d021d9cf54eecedc3287a62f2
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46872
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Auto-Submit: Ben Clayton <bclayton@google.com>
Registering a new Symbol with the NameFor() of the source symbol creates
a new *named* symbol. When mixing these with unnamed symbols we can have
collisions.
Update CloneContext::Clone(Symbol) to properly clone unnamed symbols.
Update (most) the transforms to ctx.Clone() the symbols instead of
registering the names directly.
Fix up the tests where the symbol IDs have changed.
Note: We can still have symbol collisions if a program is authored with
identifiers like 'tint_symbol_3'. This will be fixed up in a later
change.
Change-Id: I0ce559644da3d60e1060f2eef185fa55ae284521
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46866
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Each AST node must be unique.
Having diamonds in the AST causes all sorts of exciting bugs in the resolver and later transforms.
Bug: tint:469
Change-Id: I5dc43fef71a200632b3e8e8add77ec0537b01cd2
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46870
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
An tint-internal decoration used to add metadata between a sanitizer transform and a backend.
Will be used for declaring backend-specific intrinsic calls.
Change-Id: Ia05ba7dada0148de2d490605ba4d15c593075356
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46868
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
Derives from semantic::Expression.
Maps to ast::IdentifierExpressions that resolve to a variable.
Breaks pure-immutability of semantic::Variable, as we have discussed in the past.
Change-Id: I362d4d1ed61291282a60626b84fb15566655fb14
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46627
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
* Moved global variable resolving logic to new function Resolver::GlobalVariable, and moved validation logic there.
* Moved global variable-related tests to resolver tests.
* Fixed many tests that started failing after this change, mainly because many globals were declared with no storage class. I set most of these to "Input".
Bug: tint:642
Change-Id: I0f8ea2091ed2bb3faa358f9497cd884b2994a40f
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/46940
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Ben Clayton
David Neto
Arman Uguray
Antonio Maiorano
James Price