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>
[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>
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>
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>
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>
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>
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>
