This was only called for function-scope variable declarations.
In calling this, there were inevitable tests failing, which have now been fixed.
Added a test for the single runtime-array-length validation rule that this function was checking.
Fixed: tint:345
Change-Id: Ic453c38158c1290a5e1ef6de56af0c621d97982a
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48381
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: David Neto <dneto@google.com>
Fixed: tint:94
Change-Id: I1d3e512c030ec16031b8c8fcfbde0cd1db5d1ea4
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48380
Reviewed-by: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Handle the case where the OpBranchConditional in a loop header
branches to two distinct blocks inside the loop construct.
This is an if-selection in disguise.
Create an kIfSelection with the same set of blocks as the kLoop,
and with the continue target as the merge.
Fixed: tint:524
Change-Id: I5150d19a2b4388da409e2da6e68ffafdc5d21a9a
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47560
Commit-Queue: David Neto <dneto@google.com>
Auto-Submit: David Neto <dneto@google.com>
Reviewed-by: Alan Baker <alanbaker@google.com>
Fixes "error : use of identifier 'Node' found via unqualified lookup
into dependent bases of class templates is a Microsoft extension
[-Werror,-Wmicrosoft-template]"
Change-Id: Id54cdff24189b73c625f951ae369ec292be4c81b
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48340
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
AST nodes must not be shared. Diamonds in the AST will cause all sorts
of exciting, non trivial bugs.
All AST nodes must be reached by the Resolver. There are two common
reasons why they may not be:
(a) They were constructed and not attached to the AST. Several
transforms scan the full list of constructed AST nodes to find nodes
of a given type. Having detached nodes will likely cause bugs in
these transforms. Detached nodes is also just a waste of memory.
(b) They are attached to the AST, but the resolver did not traverse
them. Having the resolver skip over parts of the AST will fail to
catch validation issues, and will leave semantic gaps, likely
breaking downstream logic.
Bug: tint:469
Change-Id: I143b84fd830699f874d2936146f0e93197db610c
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47778
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
By making nodes reachable, the resolver has now caught a whole lot of additional problems, which have been fixed in this CL.
Some of these broken tests were attempting to use private and workgroup variables as function-scope declarations.
This is not legal, and these have been moved to module-scope variables.
Bug: tint:469
Change-Id: I1fc3a10fa0e39e1c290a13323277d6e9257778c4
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48048
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Check that pre-clone objects are owned by the source program.
Check that post-clone object are owned by the target builder.
Fixed: tint:469
Change-Id: Idd0eeb8dfb386e295b66b4b6621cc13dc1a30786
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48260
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
By making nodes reachable, the resolver has now caught a whole lot of additional problems, which have been fixed in this CL.
Some of these broken tests were attempting to use private and workgroup variables as function-scope declarations.
This is not legal, and these have been moved to module-scope variables.
Bug: tint:469
Change-Id: I8c91ace57701e9bec1f706eed0d9de7507c4b65a
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48223
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
This makes the .cc files match the header file name, which allows for
the text editor "header flip" feature to actually work. The reason these
files were named this way was because GN doesn't allow name conflicts in
the same source set, despite the files being in different directories.
This change splits the files into different source sets. To do so, we
use GN templates, which also reduces duplication in each target
definition.
Bug: tint:724
Change-Id: I9a7ed3912e4b85b2b38d360805203f3488b86c4c
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48160
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Implement emission of module scope variables of the private and workgroup storage classes.
Fix tests that were incorrectly emitting these as function-scope variables.
Change-Id: I509a7388794f4a57b06a3859d10afa2611d84991
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48222
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
By making nodes reachable, the resolver has now caught a whole lot of additional problems, which have been fixed in this CL.
Some of these broken tests were attempting to use private and workgroup variables as function-scope declarations.
This is not legal, and these have been moved to module-scope variables.
Bug: tint:469
Change-Id: If5e812da3f62f096e344c50782ab0f465ae74ea3
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48224
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
By making nodes reachable, the resolver has now caught a whole lot of additional problems, which have been fixed in this CL.
Some of these broken tests were attempting to use private and workgroup variables as function-scope declarations.
This is not legal, and these have been moved to module-scope variables.
Bug: tint:469
Change-Id: I09c1a8e72a33d3e4df13554e8b07d0a169fcf575
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48226
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
Module scope variables of the private and workgroup storage class are
not currently implemeted.
Change-Id: Ibb73faec5b5bcb11199e07ba4d7aa4df9c875450
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48221
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Detached AST nodes will become an ICE, so only create nodes if they are going to be used.
Bug: tint:469
Change-Id: I29b3275a355e30c934e6e508185a19981dcc8a42
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48050
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
EmitControlBarrier() was calling MakeOperand() to create ast::Expressions that held a ScalarConstructorExpression, which held a IntLiteral.
The literal value was then taken, and the rest was discarded.
Detached AST nodes will become an ICE, so do the work to find the literal value.
Bug: tint:469
Change-Id: I522bfe8db84e853e189c714b18598feb0d49e58b
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48049
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
Instead of directly fowarding to ReturnStatement, pass the parameter through `Expr()`.
Also add a no-arg overload.
This means we can write `Return(1);`, `Return("a")`, `Return()`
Change-Id: I8f5df630f540f9cecdf82d24e2810a0844e025e8
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48046
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
This is not valid. Will become an ICE.
Bug: tint:469
Change-Id: I02c0eea16daf7d83f4d6c251e06d9cac0dfd7ce4
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47777
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
The CloneContext will currently de-duplicate any pointers that are cloned multiple times, however this isn't ideal behavior for AST nodes.
AST nodes must be unique in the tree, so a non-transform clone of the AST from one program to another should only ever Clone() called once per node. Hitting the map for these is inefficent, worse still, there are cases in the transforms where we actually *want* to create N copies of the same source node. The current implementation makes this impossible.
This change introduces ShareableCloneable, a new base class that derives from Cloneable.
Repeated calls to CloneContext::Clone() for Cloneable pointer types will now produce a new, unique instance per call.
Repeated calls to CloneContext::Clone() for ShareableCloneable pointer types will de-duplicate as before.
type::Type objects are shared, want deduplication while cloning, so now derive from ShareableCloneable.
ast::Node continues to derive from Cloneable.
Bug tint:469
Change-Id: I5ca906d507de6271d5d715cfdc962a55b721e821
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47776
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
This is a trivial mapping to/from WGSL in all cases.
Bug: tint:478
Change-Id: I7f21a2392543a880906b54fddbdb8bbd149a526e
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48140
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>
Kokoro: Kokoro <noreply+kokoro@google.com>
[Is,As] contain a static_assert() that checks a cast is actually possible (TO -> FROM share a common base class).
This has been extremely valuable - it's caught numerious impossible casts due to stupid mistakes - however it makes certain generic templates impossible to write.
Add a compile-time FLAGS argument to Is() and As(), which accepts a new kDontErrorOnImpossibleCast flag.
When specified, this static_assert will always pass, allowing impossible casts to be attempted.
Change-Id: I5ff434b329c04d007f4e6976301bf30c73ab3f8d
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47775
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Error: explicitly-defaulted constructor cannot have default arguments
This broke chromium, and needs fixing before we can restart the autorollers.
Change-Id: I4b04dd127b5fb7cf84e5782e07bcf68b2befc904
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48041
Auto-Submit: Ben Clayton <bclayton@google.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Fixed: tint:673
Change-Id: I5a58d9e504446ccff724e368b5ea2cf835d2271b
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47768
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Ryan Harrison <rharrison@chromium.org>
This test landed simultaneously with a change that checks that storage buffers have an access qualifier, leading to broken tests.
Also fix lint issues that have crept in.
Change-Id: I4a7f0bc67d8d3e7170ddea117342a510741a495c
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48040
Auto-Submit: Ben Clayton <bclayton@google.com>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Almost all transforms should clone all symbols before doing any work,
to avoid any newly created symbols clashing with existing symbols the
source program and causing them to be renamed.
The Renamer is the exception to this, and so an optional flag is used
to prevent automatic cloning of symbols for this transform.
Bug: dawn:758
Change-Id: I84527a352825b2eaa43eabe225beb9e0999bf048
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/48000
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Ben Clayton <bclayton@chromium.org>
Produces ugly code for the common case.
Change-Id: I8e5b1215e19fc6461dc40b8a91922db25f9cbd76
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47764
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: James Price <jrprice@google.com>
https://gpuweb.github.io/gpuweb/wgsl/#variable-declaration
Variables in the storage storage class and variables with a storage
texture type must have an access attribute applied to the store type.
https://gpuweb.github.io/gpuweb/wgsl/#module-scope-variables
A variable in the storage storage class is a storage buffer variable. Its
store type must be a host-shareable structure type with block attribute,
satisfying the storage class constraints.
Fixup tests, including those that were producing warnings about `var <in>`
The WGSL writer seems to want to put a newline after every decoration block, leading to some ugly output. I'll fix this as a separate change.
Fixes: tint:531
Fixes: tint:692
Change-Id: If09d987477247ab4a7c635f6ee6e616a06061515
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47763
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Reviewed-by: Ryan Harrison <rharrison@chromium.org>
The canonical type was stopping at the first encountered type::AccessControl, which meant the alias in access<alias<i32>> was not being unwrapped.
Bug: tint:705
Change-Id: Idcbd824808d8ee3098fb1861add5014d7d46b0ad
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47762
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Move transform::Transform::Output to transform::Output.
There's no need for this to be an nested class, it stutters, and it also
prevents Dawn from forward declaring it.
Add move assignment operator to DataMap.
Change-Id: Ibe1af03abc1a872790d20ee6ec8cf18a511ea0b4
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47772
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Adds additional modes to the Renamer for renaming just HLSL or MSL
keywords. The sanitizers no longer rename reserved keywords
automatically, which isn't usually necessary anyway since Dawn renames
all symbols.
The tint executable automatically renames reserved keywords when
targeting HLSL and MSL. It now also has an option to rename
everything, which is useful for testing the renamer.
Change-Id: Idbfd53226805e851050024402be8d8f251b88707
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47960
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Auto-Submit: James Price <jrprice@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@chromium.org>
type::Array was the only type with special handling in that we'd resolve
all array types first in ResolverInternal, then proceed with resolving
the AST in order of declaration. This CL removes this special handling,
and instead, we now process Arrays as we process ast::Variables of
array type. This change also allows us to pass down a Source location
for validation messages when processing Arrays.
Updated some Builder tests that weren't creating a variable of the array
type they declared.
Bug: tint:707
Change-Id: I8483b3a979bc1e5e04feb1ca4d281e96e9e654be
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47825
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: Ben Clayton <bclayton@chromium.org>
Assert in each AST constructor that symbols belong to the program of the parent.
Bug: tint:709
Change-Id: I82ae9b23c88e89714a44e057a0272f0293385aaf
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47624
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Assert in each AST constructor that child nodes belong to the program of the parent.
Bug: tint:709
Change-Id: Icc89b69691d099e358ff632a0ca6fd7943cb0193
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47623
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Don't end the EXPECT_THAT() with `<< ToString(p->builder(), fe.ast_body());`
This string is already emitted in the error, and calling ast_body() a second time triggers an assert as the builder is already finalized.
Change-Id: Ied6aca73937e86aa6ce2d8cff8a09f67b5082349
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47767
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
HLSL interface matching rules impose additional requirements on the
order of structure members. We now sort members such that all members
with location attributes appear first (ordered by location slot),
followed by those with builtin attributes.
Fixed: tint:710
Change-Id: I90940bcb7a5b9eeb1f50f132d406d4cf74e47ea2
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47822
Commit-Queue: James Price <jrprice@google.com>
Auto-Submit: James Price <jrprice@google.com>
Reviewed-by: Ben Clayton <bclayton@chromium.org>
Fix a test that did not have the position builtin as a vertex shader
output.
Change-Id: I8dceba59d8327938e725e7d5e62b4b9c43695710
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47820
Reviewed-by: Ben Clayton <bclayton@chromium.org>
Commit-Queue: James Price <jrprice@google.com>
Auto-Submit: James Price <jrprice@google.com>
Pre-clone the program's symbols at the start of the transform,
otherwise the original program's symbols may get mangled. This causes
problems for Dawn when the entry point name is changed.
Change-Id: I414c798fb5f51afe44e8b97619f77452f97f0782
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47824
Commit-Queue: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Auto-Submit: James Price <jrprice@google.com>
Reviewed-by: Ben Clayton <bclayton@chromium.org>
Removes the need to pass the storage class to the SPIR-V reader
builtin mapping function.
Added a deprecation warning for sample_mask_{in,out}.
Bug: tint:715
Change-Id: I948ff2de2d5de7bd05e1c6ff45bd721c856900e3
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47743
Commit-Queue: James Price <jrprice@google.com>
Auto-Submit: James Price <jrprice@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Use the variable storage class to determine the correct builtin to use
in the SPIR-V generator.
Added a deprecation warning for frag_coord.
Bug: tint:714
Change-Id: I5ad4956f9345e2f39f4af16e84668dec345ac82e
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47742
Auto-Submit: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
The MSL writer GTest harness (TestHelper) now provides a function to
invoke the XCode SDK Metal compiler for the MSL output of a given
tint::Program.
The tint_unittests binary now provides the `--validate-msl` and
`--xcrun-path` command-line flags to optionally enable MSL validation
and to configure its path.
The MSL validation logic itself is conditionally compiled based on the
TINT_BUILD_MSL_WRITER define. The TINT_BUILD_* flags were previously
not propagated to the GTest binary which this CL addresses by linking
the common/public tint configs when building the tint_unittests_main
target.
Bug: tint:535
Fixed: tint:696
Change-Id: I08b1c36ba59c606ef6cffa5fa5454fd8cf8b035d
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/45800
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Arman Uguray <armansito@chromium.org>
We need to pre-clone the source program's symbols before creating the symbols `min` and `arrayLength`, otherwise the original program's symbols will be suffixed with a number to make them unique.
Fixes Dawn tests that triggered this issue.
Bug: tint:712
Change-Id: Ie1cf6cbcf2050a2ce1a94acf0ae131a06f635820
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47761
Auto-Submit: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: James Price <jrprice@google.com>
Reviewed-by: James Price <jrprice@google.com>
Instead of rolling another implementation inside GeneratorImpl.
Bug: tint:712
Change-Id: I26af0d68f6529c0c6dc45f51233f4618389edb55
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47638
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
And drop the leading understore, it's no longer needed.
Bug tint:712
Change-Id: Ic0ad304119ceb148984d2fa0a5e9e61f2c3a89fd
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47637
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
And clean up some code in the process.
Avoids potential symbol collisions. Simplifies the logic.
Bug: tint:711
Bug: tint:712
Change-Id: I7c41dff81e1fb2abfd3f5d3fecf625a27c42f14d
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47635
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
This will be used to detect accidental leaks of program objects between programs.
Bug: tint:709
Change-Id: I20f784a2c673d19a04a880b3ec91dfe2eb743bdb
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47622
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
Don't use a leading underscore in identifiers. Tint cannot parse these, they're illegal in MSL.
Bug: tint:640
Change-Id: I6e923cf3317a646cc5f4f2a10a7a522036000c70
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47634
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
And clean up some code in the process.
Avoids potential symbol collisions. Simplifies the logic.
Bug: tint:712
Change-Id: Ibce5ccbd4c7fd45d5bf29906b5a83b3637b6cdcc
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47633
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
Avoids potential symbol collisions. Simplifies the logic.
Bug: tint:712
Change-Id: I4416307b10f2dbe38964b6fd9342042c7e5505ec
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47632
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: James Price <jrprice@google.com>
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>
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>
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>
Ben Clayton
David Neto
Antonio Maiorano
James Price
Arman Uguray