From 8cb5a23afba97e0ea4dec6e96ae25ac9f1954855 Mon Sep 17 00:00:00 2001 From: James Price Date: Fri, 16 Apr 2021 10:29:54 +0000 Subject: [PATCH] Automatically clone all symbols in CloneContext() 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 Commit-Queue: Ben Clayton Reviewed-by: Ben Clayton --- src/clone_context.cc | 19 ++++--- src/clone_context.h | 11 ++-- src/clone_context_test.cc | 7 ++- src/transform/bound_array_accessors.cc | 5 -- src/transform/canonicalize_entry_point_io.cc | 4 -- src/transform/decompose_storage_access.cc | 4 -- src/transform/emit_vertex_point_size.cc | 4 -- src/transform/renamer.cc | 3 +- src/transform/vertex_pulling.cc | 4 -- .../hlsl/generator_impl_function_test.cc | 50 +++++++++---------- 10 files changed, 46 insertions(+), 65 deletions(-) diff --git a/src/clone_context.cc b/src/clone_context.cc index 587e593805..902366086a 100644 --- a/src/clone_context.cc +++ b/src/clone_context.cc @@ -26,8 +26,18 @@ namespace tint { CloneContext::ListTransforms::ListTransforms() = default; CloneContext::ListTransforms::~ListTransforms() = default; -CloneContext::CloneContext(ProgramBuilder* to, Program const* from) - : dst(to), src(from) {} +CloneContext::CloneContext(ProgramBuilder* to, + Program const* from, + bool auto_clone_symbols) + : dst(to), src(from) { + if (auto_clone_symbols) { + // Almost all transforms will want to clone all symbols before doing any + // work, to avoid any newly created symbols clashing with existing symbols + // in the source program and causing them to be renamed. + from->Symbols().Foreach([&](Symbol s, const std::string&) { Clone(s); }); + } +} + CloneContext::~CloneContext() = default; Symbol CloneContext::Clone(Symbol s) { @@ -39,11 +49,6 @@ Symbol CloneContext::Clone(Symbol s) { }); } -CloneContext& CloneContext::CloneSymbols() { - src->Symbols().Foreach([&](Symbol s, const std::string&) { Clone(s); }); - return *this; -} - void CloneContext::Clone() { dst->AST().Copy(this, &src->AST()); } diff --git a/src/clone_context.h b/src/clone_context.h index 1243e342c7..2ff66caaac 100644 --- a/src/clone_context.h +++ b/src/clone_context.h @@ -63,7 +63,10 @@ class CloneContext { /// Constructor /// @param to the target ProgramBuilder to clone into /// @param from the source Program to clone from - CloneContext(ProgramBuilder* to, Program const* from); + /// @param auto_clone_symbols clone all symbols in `from` before returning + CloneContext(ProgramBuilder* to, + Program const* from, + bool auto_clone_symbols = true); /// Destructor ~CloneContext(); @@ -150,12 +153,6 @@ class CloneContext { /// @return the cloned source Symbol Clone(Symbol s); - /// Clones all the Symbols in `src->Symbols()` into #dst. - /// This may be used to ensure that authored symbols are not suffixed with a - /// unique identifier if they collide with other symbols. - /// @returns this CloneContext so calls can be chained - CloneContext& CloneSymbols(); - /// Clones each of the elements of the vector `v` into the ProgramBuilder /// #dst. /// diff --git a/src/clone_context_test.cc b/src/clone_context_test.cc index ea6f90242e..c64ca9f5ae 100644 --- a/src/clone_context_test.cc +++ b/src/clone_context_test.cc @@ -224,7 +224,7 @@ TEST(CloneContext, CloneWithReplaceAll_Symbols) { // N: Node ProgramBuilder cloned; - auto* cloned_root = CloneContext(&cloned, &original) + auto* cloned_root = CloneContext(&cloned, &original, false) .ReplaceAll([&](Symbol sym) { auto in = original.Symbols().NameFor(sym); auto out = "transformed<" + in + ">"; @@ -435,7 +435,7 @@ TEST(CloneContext, CloneNewUnnamedSymbols) { Program original(std::move(builder)); ProgramBuilder cloned; - CloneContext ctx(&cloned, &original); + CloneContext ctx(&cloned, &original, false); Symbol new_x = cloned.Symbols().New(); Symbol new_a = ctx.Clone(old_a); Symbol new_y = cloned.Symbols().New(); @@ -463,7 +463,7 @@ TEST(CloneContext, CloneNewSymbols) { Program original(std::move(builder)); ProgramBuilder cloned; - CloneContext ctx(&cloned, &original); + CloneContext ctx(&cloned, &original, false); Symbol new_x = cloned.Symbols().New("a"); Symbol new_a = ctx.Clone(old_a); Symbol new_y = cloned.Symbols().New("b"); @@ -492,7 +492,6 @@ TEST(CloneContext, CloneNewSymbols_AfterCloneSymbols) { ProgramBuilder cloned; CloneContext ctx(&cloned, &original); - ctx.CloneSymbols(); Symbol new_x = cloned.Symbols().New("a"); Symbol new_a = ctx.Clone(old_a); Symbol new_y = cloned.Symbols().New("b"); diff --git a/src/transform/bound_array_accessors.cc b/src/transform/bound_array_accessors.cc index 3f6416b42c..e66866d352 100644 --- a/src/transform/bound_array_accessors.cc +++ b/src/transform/bound_array_accessors.cc @@ -29,11 +29,6 @@ BoundArrayAccessors::~BoundArrayAccessors() = default; Output BoundArrayAccessors::Run(const Program* in, const DataMap&) { ProgramBuilder out; CloneContext ctx(&out, in); - - // Start by cloning all the symbols. This ensures that the authored symbols - // won't get renamed if they collide with new symbols below. - ctx.CloneSymbols(); - ctx.ReplaceAll([&](ast::ArrayAccessorExpression* expr) { return Transform(expr, &ctx); }); diff --git a/src/transform/canonicalize_entry_point_io.cc b/src/transform/canonicalize_entry_point_io.cc index 622b6b2fa4..06bb158bc2 100644 --- a/src/transform/canonicalize_entry_point_io.cc +++ b/src/transform/canonicalize_entry_point_io.cc @@ -62,10 +62,6 @@ Output CanonicalizeEntryPointIO::Run(const Program* in, const DataMap&) { ProgramBuilder out; CloneContext ctx(&out, in); - // Start by cloning all the symbols. This ensures that the authored symbols - // won't get renamed if they collide with new symbols below. - ctx.CloneSymbols(); - // Strip entry point IO decorations from struct declarations. // TODO(jrprice): This code is duplicated with the SPIR-V transform. for (auto* ty : ctx.src->AST().ConstructedTypes()) { diff --git a/src/transform/decompose_storage_access.cc b/src/transform/decompose_storage_access.cc index d154514663..a5c9e09a16 100644 --- a/src/transform/decompose_storage_access.cc +++ b/src/transform/decompose_storage_access.cc @@ -613,10 +613,6 @@ Output DecomposeStorageAccess::Run(const Program* in, const DataMap&) { ProgramBuilder out; CloneContext ctx(&out, in); - // Start by cloning all the symbols. This ensures that the authored symbols - // won't get renamed if they collide with new symbols below. - ctx.CloneSymbols(); - auto& sem = ctx.src->Sem(); State state; diff --git a/src/transform/emit_vertex_point_size.cc b/src/transform/emit_vertex_point_size.cc index 676d400a3e..048d23a7de 100644 --- a/src/transform/emit_vertex_point_size.cc +++ b/src/transform/emit_vertex_point_size.cc @@ -35,10 +35,6 @@ Output EmitVertexPointSize::Run(const Program* in, const DataMap&) { CloneContext ctx(&out, in); - // Start by cloning all the symbols. This ensures that the authored symbols - // won't get renamed if they collide with new symbols below. - ctx.CloneSymbols(); - Symbol pointsize = out.Symbols().New("tint_pointsize"); // Declare the pointsize builtin output variable. diff --git a/src/transform/renamer.cc b/src/transform/renamer.cc index 9ed474320f..a5a68562ec 100644 --- a/src/transform/renamer.cc +++ b/src/transform/renamer.cc @@ -848,7 +848,8 @@ Renamer::~Renamer() = default; Output Renamer::Run(const Program* in, const DataMap&) { ProgramBuilder out; - CloneContext ctx(&out, in); + // Disable auto-cloning of symbols, since we want to rename them. + CloneContext ctx(&out, in, false); // Swizzles and intrinsic calls need to keep their symbols preserved. std::unordered_set preserve; diff --git a/src/transform/vertex_pulling.cc b/src/transform/vertex_pulling.cc index a34acf37c7..7b5d1a2112 100644 --- a/src/transform/vertex_pulling.cc +++ b/src/transform/vertex_pulling.cc @@ -414,10 +414,6 @@ Output VertexPulling::Run(const Program* in, const DataMap& data) { CloneContext ctx(&out, in); - // Start by cloning all the symbols. This ensures that the authored symbols - // won't get renamed if they collide with new symbols below. - ctx.CloneSymbols(); - State state{ctx, cfg}; state.FindOrInsertVertexIndexIfUsed(); state.FindOrInsertInstanceIndexIfUsed(); diff --git a/src/writer/hlsl/generator_impl_function_test.cc b/src/writer/hlsl/generator_impl_function_test.cc index cdadf42b8f..c3fa8c43e5 100644 --- a/src/writer/hlsl/generator_impl_function_test.cc +++ b/src/writer/hlsl/generator_impl_function_test.cc @@ -126,10 +126,10 @@ struct tint_symbol_2 { float value : SV_Target1; }; -tint_symbol_2 frag_main(tint_symbol_1 tint_symbol_3) { - const float foo = tint_symbol_3.foo; - const tint_symbol_2 tint_symbol = {foo}; - return tint_symbol; +tint_symbol_2 frag_main(tint_symbol_1 tint_symbol) { + const float foo = tint_symbol.foo; + const tint_symbol_2 tint_symbol_3 = {foo}; + return tint_symbol_3; } )"); @@ -160,10 +160,10 @@ struct tint_symbol_2 { float value : SV_Depth; }; -tint_symbol_2 frag_main(tint_symbol_1 tint_symbol_3) { - const float4 coord = tint_symbol_3.coord; - const tint_symbol_2 tint_symbol = {coord.x}; - return tint_symbol; +tint_symbol_2 frag_main(tint_symbol_1 tint_symbol) { + const float4 coord = tint_symbol.coord; + const tint_symbol_2 tint_symbol_3 = {coord.x}; + return tint_symbol_3; } )"); @@ -210,7 +210,7 @@ TEST_F(HlslGeneratorImplTest_Function, float col1; float col2; }; -struct tint_symbol_1 { +struct tint_symbol { float col1 : TEXCOORD1; float col2 : TEXCOORD2; }; @@ -219,10 +219,10 @@ struct tint_symbol_3 { float col2 : TEXCOORD2; }; -tint_symbol_1 vert_main() { - const Interface tint_symbol_1_1 = {0.5f, 0.25f}; - const tint_symbol_1 tint_symbol = {tint_symbol_1_1.col1, tint_symbol_1_1.col2}; - return tint_symbol; +tint_symbol vert_main() { + const Interface tint_symbol_1 = {0.5f, 0.25f}; + const tint_symbol tint_symbol_4 = {tint_symbol_1.col1, tint_symbol_1.col2}; + return tint_symbol_4; } void frag_main(tint_symbol_3 tint_symbol_2) { @@ -278,28 +278,28 @@ TEST_F(HlslGeneratorImplTest_Function, EXPECT_EQ(result(), R"(struct VertexOutput { float4 pos; }; -struct tint_symbol_2 { +struct tint_symbol { float4 pos : SV_Position; }; -struct tint_symbol_2_1 { +struct tint_symbol_2 { float4 pos : SV_Position; }; VertexOutput foo(float x) { - const VertexOutput tint_symbol = {float4(x, x, x, 1.0f)}; - return tint_symbol; + const VertexOutput tint_symbol_4 = {float4(x, x, x, 1.0f)}; + return tint_symbol_4; } -tint_symbol_2 vert_main1() { - const VertexOutput tint_symbol_1_1 = {foo(0.5f)}; - const tint_symbol_2 tint_symbol_1 = {tint_symbol_1_1.pos}; - return tint_symbol_1; +tint_symbol vert_main1() { + const VertexOutput tint_symbol_1 = {foo(0.5f)}; + const tint_symbol tint_symbol_5 = {tint_symbol_1.pos}; + return tint_symbol_5; } -tint_symbol_2_1 vert_main2() { - const VertexOutput tint_symbol_3_1 = {foo(0.25f)}; - const tint_symbol_2_1 tint_symbol_3 = {tint_symbol_3_1.pos}; - return tint_symbol_3; +tint_symbol_2 vert_main2() { + const VertexOutput tint_symbol_3 = {foo(0.25f)}; + const tint_symbol_2 tint_symbol_6 = {tint_symbol_3.pos}; + return tint_symbol_6; } )");