resolver: Allow parameters to shadow globals

In https://dawn-review.googlesource.com/c/tint/+/62444 the Resolver validated that there are no parameters of the same function with the same name, but this also introduced validation that errors if parameters shadow a module-scope variable.

The WGSL spec allows for shadowing, but Tint so far has not implemented this support.

There are transforms that generate functions that presume parameter <-> module-scope variable shadowing is okay. DecomposeMemoryAccess is one of these.

This fixes those transforms which could generate programs that fail validation.

Bug: chromium:1242330
Fixed: tint:1136
Change-Id: Id6ec59bbdb398b3b2a23312115a7c1dadf433e98
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/62900
Reviewed-by: James Price <jrprice@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>

src/resolver/function_validation_test.cc

@@ -58,7 +58,7 @@ TEST_F(ResolverFunctionValidationTest, ParameterNamesMustBeUnique_fail) {
 
   EXPECT_FALSE(r()->Resolve());
   EXPECT_EQ(r()->error(),
-            R"(12:34 error: redefinition of 'common_name'
+            R"(12:34 error: redefinition of parameter 'common_name'
 56:78 note: previous definition is here)");
 }
 
@@ -72,6 +72,17 @@ TEST_F(ResolverFunctionValidationTest, ParameterNamesMustBeUnique_pass) {
   EXPECT_EQ(r()->error(), "");
 }
 
+TEST_F(ResolverFunctionValidationTest,
+       ParameterNamesMustBeUniqueShadowsGlobal_pass) {
+  // var<private> common_name : f32;
+  // fn func(common_name : f32) { }
+  Global("common_name", ty.f32(), ast::StorageClass::kPrivate);
+  Func("func", {Param("common_name", ty.f32())}, ty.void_(), {});
+
+  EXPECT_TRUE(r()->Resolve());
+  EXPECT_EQ(r()->error(), "");
+}
+
 TEST_F(ResolverFunctionValidationTest,
        VoidFunctionEndWithoutReturnStatement_Pass) {
   // fn func { var a:i32 = 2; }

src/resolver/resolver.cc

@@ -1696,11 +1696,18 @@ bool Resolver::Function(ast::Function* func) {
 
   variable_stack_.push_scope();
   uint32_t parameter_index = 0;
+  std::unordered_map<Symbol, Source> parameter_names;
   for (auto* param : func->params()) {
     Mark(param);
 
-    if (!ValidateNoDuplicateDefinition(param->symbol(), param->source())) {
+    {  // Check the parameter name is unique for the function
-      return false;
+      auto emplaced = parameter_names.emplace(param->symbol(), param->source());
+      if (!emplaced.second) {
+        auto name = builder_->Symbols().NameFor(param->symbol());
+        AddError("redefinition of parameter '" + name + "'", param->source());
+        AddNote("previous definition is here", emplaced.first->second);
+        return false;
+      }
     }
 
     auto* param_info =

test/bug/tint/1136.wgsl

@@ -0,0 +1,3 @@
+[[block]] struct Buffer { data: u32; };
+[[group(0), binding(0)]] var<storage, read_write> buffer: Buffer;
+fn main() { buffer.data = buffer.data + 1u;  }

test/bug/tint/1136.wgsl.expected.hlsl

@@ -0,0 +1,10 @@
+[numthreads(1, 1, 1)]
+void unused_entry_point() {
+  return;
+}
+
+RWByteAddressBuffer buffer : register(u0, space0);
+
+void main() {
+  buffer.Store(0u, asuint((buffer.Load(0u) + 1u)));
+}

test/bug/tint/1136.wgsl.expected.msl

@@ -0,0 +1,11 @@
+#include <metal_stdlib>
+
+using namespace metal;
+struct Buffer {
+  /* 0x0000 */ uint data;
+};
+
+void tint_symbol_1(device Buffer& tint_symbol) {
+  tint_symbol.data = (tint_symbol.data + 1u);
+}
+

test/bug/tint/1136.wgsl.expected.spvasm

@@ -0,0 +1,40 @@
+; SPIR-V
+; Version: 1.3
+; Generator: Google Tint Compiler; 0
+; Bound: 18
+; Schema: 0
+               OpCapability Shader
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint GLCompute %unused_entry_point "unused_entry_point"
+               OpExecutionMode %unused_entry_point LocalSize 1 1 1
+               OpName %Buffer "Buffer"
+               OpMemberName %Buffer 0 "data"
+               OpName %buffer "buffer"
+               OpName %unused_entry_point "unused_entry_point"
+               OpName %main "main"
+               OpDecorate %Buffer Block
+               OpMemberDecorate %Buffer 0 Offset 0
+               OpDecorate %buffer DescriptorSet 0
+               OpDecorate %buffer Binding 0
+       %uint = OpTypeInt 32 0
+     %Buffer = OpTypeStruct %uint
+%_ptr_StorageBuffer_Buffer = OpTypePointer StorageBuffer %Buffer
+     %buffer = OpVariable %_ptr_StorageBuffer_Buffer StorageBuffer
+       %void = OpTypeVoid
+          %5 = OpTypeFunction %void
+     %uint_0 = OpConstant %uint 0
+%_ptr_StorageBuffer_uint = OpTypePointer StorageBuffer %uint
+     %uint_1 = OpConstant %uint 1
+%unused_entry_point = OpFunction %void None %5
+          %8 = OpLabel
+               OpReturn
+               OpFunctionEnd
+       %main = OpFunction %void None %5
+         %10 = OpLabel
+         %13 = OpAccessChain %_ptr_StorageBuffer_uint %buffer %uint_0
+         %14 = OpAccessChain %_ptr_StorageBuffer_uint %buffer %uint_0
+         %15 = OpLoad %uint %14
+         %17 = OpIAdd %uint %15 %uint_1
+               OpStore %13 %17
+               OpReturn
+               OpFunctionEnd

test/bug/tint/1136.wgsl.expected.wgsl

@@ -0,0 +1,10 @@
+[[block]]
+struct Buffer {
+  data : u32;
+};
+
+[[group(0), binding(0)]] var<storage, read_write> buffer : Buffer;
+
+fn main() {
+  buffer.data = (buffer.data + 1u);
+}