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>
diff --git a/src/tint/reader/spirv/function.cc b/src/tint/reader/spirv/function.cc index 4b7b3d3..e133e58 100644 --- a/src/tint/reader/spirv/function.cc +++ b/src/tint/reader/spirv/function.cc
@@ -2557,6 +2557,8 @@ } 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 @@ switch (inst->opcode()) { case SpvOpVariable: { // This occurs for module-scope variables. - auto name = namer_.Name(inst->result_id()); - return TypedExpression{ - parser_impl_.ConvertType(inst->type_id(), PtrAs::Ref), - create<ast::IdentifierExpression>(Source{}, builder_.Symbols().Register(name))}; + auto name = namer_.Name(id); + // Construct the reference type, mapping storage class correctly. + const auto* type = + 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 @@ 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 @@ } 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 @@ if (!expr) { return false; } - expr.type = RemapAddressSpace(expr.type, result_id); return EmitConstDefOrWriteToHoistedVar(inst, expr); } @@ -3777,20 +3784,6 @@ 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 @@ 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 @@ // 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 @@ 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 @@ ++index; auto& info = def_info_[result_id]; - // Determine address space for pointer values. Do this in order because - // we might rely on the address space 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) { + // 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. 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 = - GetAddressSpaceForPointerValue(inst.GetSingleWordInOperand(0)); + info->pointer = GetPointerInfo(inst.GetSingleWordInOperand(0)); break; default: return Fail() << "pointer defined in function from unknown opcode: " @@ -4846,32 +4839,71 @@ 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; - if (candidate != ast::AddressSpace::kInvalid) { - return candidate; + const auto& info = where->second; + if (info->inst.opcode() == SpvOpVariable) { + // 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(); - if (type_id) { - auto* ast_type = parser_impl_.ConvertType(type_id); - if (auto* ptr = As<Pointer>(ast_type)) { - return ptr->address_space; - } - } - return ast::AddressSpace::kInvalid; + const auto* inst = def_use_mgr_->GetDef(id); + TINT_ASSERT(Reader, inst); + return get_from_root_identifier(*inst); } -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 - // buffer pointer. - const auto addr_space = GetAddressSpaceForPointerValue(result_id); - if (ast_ptr_type->address_space != addr_space) { - return ty_.Pointer(ast_ptr_type->type, addr_space); - } + const auto pi = GetPointerInfo(result_id); + // TODO(crbug.com/tin/t1041): also do access mode + return ty_.Pointer(ast_ptr_type->type, pi.address_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; }
diff --git a/src/tint/reader/spirv/function.h b/src/tint/reader/spirv/function.h index dc6002e..528799a 100644 --- a/src/tint/reader/spirv/function.h +++ b/src/tint/reader/spirv/function.h
@@ -334,7 +334,8 @@ /// 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 @@ /// @returns false on failure bool RegisterLocallyDefinedValues(); - /// Returns the Tint address space for the given SPIR-V ID that is a - /// pointer value. + /// Returns the pointer information needed for the given SPIR-V ID. + /// 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 - ast::AddressSpace GetAddressSpaceForPointerValue(uint32_t id); + /// @returns the associated Pointer info + DefInfo::Pointer GetPointerInfo(uint32_t id); - /// Remaps the address space for the type of a locally-defined value, - /// if necessary. If it's not a pointer type, or if its address space - /// already matches, then the result is a copy of the `type` argument. + /// Remaps the address space and access mode for the type of a + /// locally-defined value, if necessary. If it's not a pointer or reference + /// 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 @@ /// @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
diff --git a/src/tint/reader/spirv/function_memory_test.cc b/src/tint/reader/spirv/function_memory_test.cc index dbf9219..a64aac5 100644 --- a/src/tint/reader/spirv/function_memory_test.cc +++ b/src/tint/reader/spirv/function_memory_test.cc
@@ -938,6 +938,43 @@ )")); } +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 @@ 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