spirv-reader: Track storage class for pointer/ref values

Fixed: tint:1041 tint:1648
Change-Id: I28c6677e0ef3f96902f4f9ced030c2280a17c247
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/104762
Auto-Submit: David Neto <dneto@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>

src/tint/reader/spirv/function.cc

@@ -2557,6 +2557,8 @@ TypedExpression FunctionEmitter::MakeExpression(uint32_t id) {
     }
     auto type_it = identifier_types_.find(id);
     if (type_it != identifier_types_.end()) {
+        // We have a local named definition: function parameter, let, or var
+        // declaration.
         auto name = namer_.Name(id);
         auto* type = type_it->second;
         return TypedExpression{
@@ -2585,10 +2587,13 @@ TypedExpression FunctionEmitter::MakeExpression(uint32_t id) {
     switch (inst->opcode()) {
         case SpvOpVariable: {
             // This occurs for module-scope variables.
-            auto name = namer_.Name(inst->result_id());
+            auto name = namer_.Name(id);
-            return TypedExpression{
+            // Construct the reference type, mapping storage class correctly.
-                parser_impl_.ConvertType(inst->type_id(), PtrAs::Ref),
+            const auto* type =
-                create<ast::IdentifierExpression>(Source{}, builder_.Symbols().Register(name))};
+                RemapPointerProperties(parser_impl_.ConvertType(inst->type_id(), PtrAs::Ref), id);
+            // TODO(crbug.com/tint/1041): Fix access mode
+            return TypedExpression{type, create<ast::IdentifierExpression>(
+                                             Source{}, builder_.Symbols().Register(name))};
         }
         case SpvOpUndef:
             // Substitute a null value for undef.
@@ -3356,11 +3361,13 @@ bool FunctionEmitter::EmitStatementsInBasicBlock(const BlockInfo& block_info,
     for (auto id : sorted_by_index(block_info.hoisted_ids)) {
         const auto* def_inst = def_use_mgr_->GetDef(id);
         TINT_ASSERT(Reader, def_inst);
-        auto* storage_type = RemapAddressSpace(parser_impl_.ConvertType(def_inst->type_id()), id);
+        // Compute the store type.  Pointers are not storable, so there is
+        // no need to remap pointer properties.
+        auto* store_type = parser_impl_.ConvertType(def_inst->type_id());
         AddStatement(create<ast::VariableDeclStatement>(
-            Source{}, parser_impl_.MakeVar(id, ast::AddressSpace::kNone, storage_type, nullptr,
+            Source{}, parser_impl_.MakeVar(id, ast::AddressSpace::kNone, store_type, nullptr,
                                            AttributeList{})));
-        auto* type = ty_.Reference(storage_type, ast::AddressSpace::kNone);
+        auto* type = ty_.Reference(store_type, ast::AddressSpace::kNone);
         identifier_types_.emplace(id, type);
     }
 
@@ -3449,6 +3456,7 @@ bool FunctionEmitter::EmitConstDefinition(const spvtools::opt::Instruction& inst
     }
 
     expr = AddressOfIfNeeded(expr, &inst);
+    expr.type = RemapPointerProperties(expr.type, inst.result_id());
     auto* let = parser_impl_.MakeLet(inst.result_id(), expr.type, expr.expr);
     if (!let) {
         return false;
@@ -3720,7 +3728,6 @@ bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) {
             if (!expr) {
                 return false;
             }
-            expr.type = RemapAddressSpace(expr.type, result_id);
             return EmitConstDefOrWriteToHoistedVar(inst, expr);
         }
 
@@ -3777,20 +3784,6 @@ TypedExpression FunctionEmitter::MakeOperand(const spvtools::opt::Instruction& i
     return parser_impl_.RectifyOperandSignedness(inst, std::move(expr));
 }
 
-TypedExpression FunctionEmitter::InferFunctionAddressSpace(TypedExpression expr) {
-    TypedExpression result(expr);
-    if (const auto* ref = expr.type->UnwrapAlias()->As<Reference>()) {
-        if (ref->address_space == ast::AddressSpace::kNone) {
-            expr.type = ty_.Reference(ref->type, ast::AddressSpace::kFunction);
-        }
-    } else if (const auto* ptr = expr.type->UnwrapAlias()->As<Pointer>()) {
-        if (ptr->address_space == ast::AddressSpace::kNone) {
-            expr.type = ty_.Pointer(ptr->type, ast::AddressSpace::kFunction);
-        }
-    }
-    return expr;
-}
-
 TypedExpression FunctionEmitter::MaybeEmitCombinatorialValue(
     const spvtools::opt::Instruction& inst) {
     if (inst.result_id() == 0) {
@@ -4350,6 +4343,10 @@ TypedExpression FunctionEmitter::MakeAccessChain(const spvtools::opt::Instructio
     const auto num_in_operands = inst.NumInOperands();
 
     bool sink_pointer = false;
+    // The current WGSL expression for the pointer, starting with the base
+    // pointer and updated as each index is incorported.  The important part
+    // is the pointee (or "store type").  The address space and access mode will
+    // be patched as needed at the very end, via RemapPointerProperties.
     TypedExpression current_expr;
 
     // If the variable was originally gl_PerVertex, then in the AST we
@@ -4418,7 +4415,7 @@ TypedExpression FunctionEmitter::MakeAccessChain(const spvtools::opt::Instructio
     // ever-deeper nested indexing expressions. Start off with an expression
     // for the base, and then bury that inside nested indexing expressions.
     if (!current_expr) {
-        current_expr = InferFunctionAddressSpace(MakeOperand(inst, 0));
+        current_expr = MakeOperand(inst, 0);
         if (current_expr.type->Is<Pointer>()) {
             current_expr = Dereference(current_expr);
         }
@@ -4533,6 +4530,7 @@ TypedExpression FunctionEmitter::MakeAccessChain(const spvtools::opt::Instructio
         GetDefInfo(inst.result_id())->sink_pointer_source_expr = current_expr;
     }
 
+    current_expr.type = RemapPointerProperties(current_expr.type, inst.result_id());
     return current_expr;
 }
 
@@ -4799,32 +4797,27 @@ bool FunctionEmitter::RegisterLocallyDefinedValues() {
             ++index;
             auto& info = def_info_[result_id];
 
-            // Determine address space for pointer values. Do this in order because
+            const auto* type = type_mgr_->GetType(inst.type_id());
-            // we might rely on the address space for a previously-visited definition.
+            if (type) {
+                // Determine address space and access mode for pointer values. Do this in
+                // order because we might rely on the storage class for a previously-visited
+                // definition.
                 // Logical pointers can't be transmitted through OpPhi, so remaining
                 // pointer definitions are SSA values, and their definitions must be
                 // visited before their uses.
-            const auto* type = type_mgr_->GetType(inst.type_id());
-            if (type) {
                 if (type->AsPointer()) {
-                    if (auto* ast_type = parser_impl_.ConvertType(inst.type_id())) {
-                        if (auto* ptr = ast_type->As<Pointer>()) {
-                            info->pointer.address_space = ptr->address_space;
-                        }
-                    }
                     switch (inst.opcode()) {
                         case SpvOpUndef:
                             return Fail() << "undef pointer is not valid: " << inst.PrettyPrint();
                         case SpvOpVariable:
-                            // Keep the default decision based on the result type.
+                            info->pointer = GetPointerInfo(result_id);
                             break;
                         case SpvOpAccessChain:
                         case SpvOpInBoundsAccessChain:
                         case SpvOpCopyObject:
                             // Inherit from the first operand. We need this so we can pick up
                             // a remapped storage buffer.
-                            info->pointer.address_space =
+                            info->pointer = GetPointerInfo(inst.GetSingleWordInOperand(0));
-                                GetAddressSpaceForPointerValue(inst.GetSingleWordInOperand(0));
                             break;
                         default:
                             return Fail() << "pointer defined in function from unknown opcode: "
@@ -4846,32 +4839,71 @@ bool FunctionEmitter::RegisterLocallyDefinedValues() {
     return true;
 }
 
-ast::AddressSpace FunctionEmitter::GetAddressSpaceForPointerValue(uint32_t id) {
+DefInfo::Pointer FunctionEmitter::GetPointerInfo(uint32_t id) {
+    // Compute the result from first principles, for a variable.
+    auto get_from_root_identifier =
+        [&](const spvtools::opt::Instruction& inst) -> DefInfo::Pointer {
+        // WGSL root identifiers (or SPIR-V "memory object declarations") are
+        // either variables or function parameters.
+        switch (inst.opcode()) {
+            case SpvOpVariable: {
+                if (const auto* module_var = parser_impl_.GetModuleVariable(id)) {
+                    return DefInfo::Pointer{module_var->declared_address_space,
+                                            module_var->declared_access};
+                }
+                // Local variables are always Function storage class, with default
+                // access mode.
+                return DefInfo::Pointer{ast::AddressSpace::kFunction, ast::Access::kUndefined};
+            }
+            case SpvOpFunctionParameter: {
+                const auto* type = As<Pointer>(parser_impl_.ConvertType(inst.type_id()));
+                // TODO(crbug.com/tint/1041): Add access mode.
+                // Using kUndefined is ok for now, since the only non-default access mode
+                // on a pointer would be for a storage buffer, and baseline SPIR-V doesn't
+                // allow passing pointers to buffers as function parameters.
+                return DefInfo::Pointer{type->address_space, ast::Access::kUndefined};
+            }
+            default:
+                break;
+        }
+        TINT_ASSERT(Reader, false && "expected a memory object declaration");
+        return {};
+    };
+
     auto where = def_info_.find(id);
     if (where != def_info_.end()) {
-        auto candidate = where->second.get()->pointer.address_space;
+        const auto& info = where->second;
-        if (candidate != ast::AddressSpace::kInvalid) {
+        if (info->inst.opcode() == SpvOpVariable) {
-            return candidate;
+            // Ignore the cache in this case and compute it from scratch.
+            // That's because for a function-scope OpVariable is a
+            // locally-defined value.  So its cache entry has been created
+            // with a default PointerInfo object, which has invalid data.
+            //
+            // Instead, you might think that we could forget this weirdness
+            // and instead have more standard cache-like behaviour. But then
+            // for non-function-scope variables we look up information
+            // from a saved ast::Var. But some builtins don't correspond
+            // to a declared ast::Var. This is simpler and more reliable.
+            return get_from_root_identifier(info->inst);
         }
+        // Use the cached value.
+        return info->pointer;
     }
-    const auto type_id = def_use_mgr_->GetDef(id)->type_id();
+    const auto* inst = def_use_mgr_->GetDef(id);
-    if (type_id) {
+    TINT_ASSERT(Reader, inst);
-        auto* ast_type = parser_impl_.ConvertType(type_id);
+    return get_from_root_identifier(*inst);
-        if (auto* ptr = As<Pointer>(ast_type)) {
-            return ptr->address_space;
-        }
-    }
-    return ast::AddressSpace::kInvalid;
 }
 
-const Type* FunctionEmitter::RemapAddressSpace(const Type* type, uint32_t result_id) {
+const Type* FunctionEmitter::RemapPointerProperties(const Type* type, uint32_t result_id) {
     if (auto* ast_ptr_type = As<Pointer>(type)) {
-        // Remap an old-style storage buffer pointer to a new-style storage
+        const auto pi = GetPointerInfo(result_id);
-        // buffer pointer.
+        // TODO(crbug.com/tin/t1041): also do access mode
-        const auto addr_space = GetAddressSpaceForPointerValue(result_id);
+        return ty_.Pointer(ast_ptr_type->type, pi.address_space);
-        if (ast_ptr_type->address_space != addr_space) {
-            return ty_.Pointer(ast_ptr_type->type, addr_space);
     }
+    if (auto* ast_ptr_type = As<Reference>(type)) {
+        const auto pi = GetPointerInfo(result_id);
+        // TODO(crbug.com/tin/t1041): also do access mode
+        return ty_.Reference(ast_ptr_type->type, pi.address_space);
     }
     return type;
 }

src/tint/reader/spirv/function.h

@@ -334,7 +334,8 @@ struct DefInfo {
         /// This is kInvalid for non-pointers.
         ast::AddressSpace address_space = ast::AddressSpace::kInvalid;
 
-        // TODO(crbug.com/tint/1041) track access mode.
+        /// The declared access mode.
+        ast::Access access = ast::kUndefined;
     };
 
     /// The expression to use when sinking pointers into their use.
@@ -619,19 +620,23 @@ class FunctionEmitter {
     /// @returns false on failure
     bool RegisterLocallyDefinedValues();
 
-    /// Returns the Tint address space for the given SPIR-V ID that is a
+    /// Returns the pointer information needed for the given SPIR-V ID.
-    /// pointer value.
+    /// Assumes the given ID yields a value of pointer type.  For IDs
+    /// corresponding to WGSL root identifiers (i.e. OpVariable or
+    /// OpFunctionParameter), the info is computed from scratch.
+    /// Otherwise, this looks up pointer info from a base pointer whose
+    /// data is cached in def_info_.
     /// @param id a SPIR-V ID for a pointer value
-    /// @returns the address space
+    /// @returns the associated Pointer info
-    ast::AddressSpace GetAddressSpaceForPointerValue(uint32_t id);
+    DefInfo::Pointer GetPointerInfo(uint32_t id);
 
-    /// Remaps the address space for the type of a locally-defined value,
+    /// Remaps the address space and access mode for the type of a
-    /// if necessary. If it's not a pointer type, or if its address space
+    /// locally-defined value, if necessary. If it's not a pointer or reference
-    /// already matches, then the result is a copy of the `type` argument.
+    /// type, then the result is a copy of the `type` argument.
     /// @param type the AST type
     /// @param result_id the SPIR-V ID for the locally defined value
     /// @returns an possibly updated type
-    const Type* RemapAddressSpace(const Type* type, uint32_t result_id);
+    const Type* RemapPointerProperties(const Type* type, uint32_t result_id);
 
     /// Marks locally defined values when they should get a 'let'
     /// definition in WGSL, or a 'var' definition at an outer scope.
@@ -1011,13 +1016,6 @@ class FunctionEmitter {
     /// @returns a new expression node
     TypedExpression MakeOperand(const spvtools::opt::Instruction& inst, uint32_t operand_index);
 
-    /// Copies a typed expression to the result, but when the type is a pointer
-    /// or reference type, ensures the address space is not defaulted.  That is,
-    /// it changes a address space of "none" to "function".
-    /// @param expr a typed expression
-    /// @results a copy of the expression, with possibly updated type
-    TypedExpression InferFunctionAddressSpace(TypedExpression expr);
-
     /// Returns an expression for a SPIR-V OpFMod instruction.
     /// @param inst the SPIR-V instruction
     /// @returns an expression

src/tint/reader/spirv/function_memory_test.cc

@@ -938,6 +938,43 @@ myvar.field1[1u] = 0u;
 )"));
 }
 
+TEST_F(SpvParserMemoryTest, RemapStorageBuffer_ThroughAccessChain_NonCascaded_UsedTwice) {
+    // Use the pointer value twice, which provokes the spirv-reader
+    // to make a let declaration for the pointer.  The storage class
+    // must be 'storage', not 'uniform'.
+    const auto assembly = OldStorageBufferPreamble() + R"(
+  %100 = OpFunction %void None %voidfn
+  %entry = OpLabel
+
+  ; the scalar element
+  %1 = OpAccessChain %ptr_uint %myvar %uint_0
+  OpStore %1 %uint_0
+  OpStore %1 %uint_0
+
+  ; element in the runtime array
+  %2 = OpAccessChain %ptr_uint %myvar %uint_1 %uint_1
+  ; Use the pointer twice
+  %3 = OpLoad %uint %2
+  OpStore %2 %uint_0
+
+  OpReturn
+  OpFunctionEnd
+)";
+    auto p = parser(test::Assemble(assembly));
+    ASSERT_TRUE(p->BuildAndParseInternalModule()) << assembly << p->error();
+    auto fe = p->function_emitter(100);
+    EXPECT_TRUE(fe.EmitBody()) << p->error();
+    auto ast_body = fe.ast_body();
+    const auto got = test::ToString(p->program(), ast_body);
+    EXPECT_THAT(got, HasSubstr(R"(let x_1 : ptr<storage, u32> = &(myvar.field0);
+*(x_1) = 0u;
+*(x_1) = 0u;
+let x_2 : ptr<storage, u32> = &(myvar.field1[1u]);
+let x_3 : u32 = *(x_2);
+*(x_2) = 0u;
+)"));
+}
+
 TEST_F(SpvParserMemoryTest, RemapStorageBuffer_ThroughAccessChain_NonCascaded_InBoundsAccessChain) {
     // Like the previous test, but using OpInBoundsAccessChain.
     const auto assembly = OldStorageBufferPreamble() + R"(
@@ -1020,56 +1057,6 @@ TEST_F(SpvParserMemoryTest, RemapStorageBuffer_ThroughCopyObject_WithoutHoisting
     p->SkipDumpingPending("crbug.com/tint/1041 track access mode in spirv-reader parser type");
 }
 
-TEST_F(SpvParserMemoryTest, RemapStorageBuffer_ThroughCopyObject_WithHoisting) {
-    // TODO(dneto): Hoisting non-storable values (pointers) is not yet supported.
-    // It's debatable whether this test should run at all.
-    // crbug.com/tint/98
-
-    // Like the previous test, but the declaration for the copy-object
-    // has its declaration hoisted.
-    const auto assembly = OldStorageBufferPreamble() + R"(
-  %bool = OpTypeBool
-  %cond = OpConstantTrue %bool
-
-  %100 = OpFunction %void None %voidfn
-
-  %entry = OpLabel
-  OpSelectionMerge %99 None
-  OpBranchConditional %cond %20 %30
-
-  %20 = OpLabel
-  %1 = OpAccessChain %ptr_uint %myvar %uint_1 %uint_1
-  ; this definintion dominates the use in %99
-  %2 = OpCopyObject %ptr_uint %1
-  OpBranch %99
-
-  %30 = OpLabel
-  OpReturn
-
-  %99 = OpLabel
-  OpStore %2 %uint_0
-  OpReturn
-
-  OpFunctionEnd
-)";
-    auto p = parser(test::Assemble(assembly));
-    ASSERT_TRUE(p->BuildAndParseInternalModule()) << assembly << p->error();
-    auto fe = p->function_emitter(100);
-    EXPECT_TRUE(fe.EmitBody()) << p->error();
-    auto ast_body = fe.ast_body();
-    EXPECT_EQ(test::ToString(p->program(), ast_body),
-              R"(var x_2 : ptr<storage, u32>;
-if (true) {
-  x_2 = &(myvar.field1[1u]);
-} else {
-  return;
-}
-x_2 = 0u;
-return;
-)") << p->error();
-    p->SkipDumpingPending("crbug.com/tint/98");
-}
-
 std::string RuntimeArrayPreamble() {
     return R"(
      OpCapability Shader