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>
By comparing the old substring to the new full body, we can do a pretty
good job at creating a new substring.
This is not an exact science. Always carefully examine the newly
generated strings for correctness.
Change-Id: I8bdf591539a32ec42d0444aa054d88a933e4d250
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47765
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
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>
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>
James Price
Ben Clayton
Antonio Maiorano
Arman Uguray
David Neto