From 2dbd556a1822f153bd93f98adbe2aef1b4fff548 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Tue, 12 Jan 2021 21:20:48 +0000 Subject: [PATCH] writers: Duplicate cube height for textureDimensions() All shader backend languages describe cubes as width / height, but give no way to query depth. WGSL however returns a vec3 for cube textures when calling textureDimensions(). As cube textures must be square (width == height == depth), just replicate the height for the depth. See https://github.com/gpuweb/gpuweb/issues/1345 Bug: tint:140 Bug: tint:437 Change-Id: I76ef18ee4bd8b53d5f9d9d3f1c10c3f7cb23e137 Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/37446 Commit-Queue: Ben Clayton Reviewed-by: David Neto --- src/writer/hlsl/generator_impl.cc | 35 ++++--- .../generator_impl_intrinsic_texture_test.cc | 24 +++-- src/writer/msl/generator_impl.cc | 68 ++++++++----- .../generator_impl_intrinsic_texture_test.cc | 10 +- src/writer/spirv/builder.cc | 99 +++++++++++++------ .../spirv/builder_intrinsic_texture_test.cc | 68 +++++++------ 6 files changed, 193 insertions(+), 111 deletions(-) diff --git a/src/writer/hlsl/generator_impl.cc b/src/writer/hlsl/generator_impl.cc index 7c36f18365..d95769fba4 100644 --- a/src/writer/hlsl/generator_impl.cc +++ b/src/writer/hlsl/generator_impl.cc @@ -745,7 +745,8 @@ bool GeneratorImpl::EmitTextureCall(std::ostream& pre, auto const kNotUsed = ast::intrinsic::TextureSignature::Parameters::kNotUsed; auto* texture = params[pidx.texture]; - auto* texture_type = texture->result_type()->UnwrapPtrIfNeeded(); + auto* texture_type = + texture->result_type()->UnwrapPtrIfNeeded()->As(); if (ident->intrinsic() == ast::Intrinsic::kTextureDimensions) { // Declare a variable to hold the texture dimensions @@ -763,17 +764,29 @@ bool GeneratorImpl::EmitTextureCall(std::ostream& pre, if (pidx.level != kNotUsed) { pre << pidx.level << ", "; } - if (auto* vec = expr->result_type()->As()) { - for (uint32_t i = 0; i < vec->size(); i++) { - if (i > 0) { - pre << ", "; - } - pre << dims << "[" << i << "]"; - } - } else { - pre << dims; + switch (texture_type->dim()) { + case ast::type::TextureDimension::kNone: + error_ = "texture dimension is kNone"; + return false; + case ast::type::TextureDimension::k1d: + case ast::type::TextureDimension::k1dArray: + pre << dims << ");"; + break; + case ast::type::TextureDimension::k2d: + case ast::type::TextureDimension::k2dArray: + pre << dims << "[0], " << dims << "[1]);"; + break; + case ast::type::TextureDimension::k3d: + pre << dims << "[0], " << dims << "[1], " << dims << "[2]);"; + break; + case ast::type::TextureDimension::kCube: + case ast::type::TextureDimension::kCubeArray: + // width == height == depth for cubes + // See https://github.com/gpuweb/gpuweb/issues/1345 + pre << dims << "[0], " << dims << "[1]);\n"; + pre << dims << "[2] = " << dims << "[1];"; // dims[2] = dims[1] + break; } - pre << ");"; // The result of the textureDimensions() call is now the temporary variable. out << dims; diff --git a/src/writer/hlsl/generator_impl_intrinsic_texture_test.cc b/src/writer/hlsl/generator_impl_intrinsic_texture_test.cc index f9d6448615..bd4029b182 100644 --- a/src/writer/hlsl/generator_impl_intrinsic_texture_test.cc +++ b/src/writer/hlsl/generator_impl_intrinsic_texture_test.cc @@ -67,10 +67,6 @@ ExpectedResult expected_texture_overload( "_tint_tmp", }; case ValidTextureOverload::kDimensions3d: - case ValidTextureOverload::kDimensionsCube: - case ValidTextureOverload::kDimensionsCubeArray: - case ValidTextureOverload::kDimensionsDepthCube: - case ValidTextureOverload::kDimensionsDepthCubeArray: case ValidTextureOverload::kDimensionsStorageRO3d: case ValidTextureOverload::kDimensionsStorageWO3d: return { @@ -79,6 +75,16 @@ ExpectedResult expected_texture_overload( "_tint_tmp[2]);", "_tint_tmp", }; + case ValidTextureOverload::kDimensionsCube: + case ValidTextureOverload::kDimensionsCubeArray: + case ValidTextureOverload::kDimensionsDepthCube: + case ValidTextureOverload::kDimensionsDepthCubeArray: + return { + "int3 _tint_tmp;\n" + "texture_tint_0.GetDimensions(_tint_tmp[0], _tint_tmp[1]);\n" + "_tint_tmp[2] = _tint_tmp[1];", + "_tint_tmp", + }; case ValidTextureOverload::kDimensions2dLevel: case ValidTextureOverload::kDimensions2dArrayLevel: case ValidTextureOverload::kDimensionsDepth2dLevel: @@ -89,14 +95,20 @@ ExpectedResult expected_texture_overload( "_tint_tmp", }; case ValidTextureOverload::kDimensions3dLevel: + return { + "int3 _tint_tmp;\n" + "texture_tint_0.GetDimensions(1, _tint_tmp[0], _tint_tmp[1], " + "_tint_tmp[2]);", + "_tint_tmp", + }; case ValidTextureOverload::kDimensionsCubeLevel: case ValidTextureOverload::kDimensionsCubeArrayLevel: case ValidTextureOverload::kDimensionsDepthCubeLevel: case ValidTextureOverload::kDimensionsDepthCubeArrayLevel: return { "int3 _tint_tmp;\n" - "texture_tint_0.GetDimensions(1, _tint_tmp[0], _tint_tmp[1], " - "_tint_tmp[2]);", + "texture_tint_0.GetDimensions(1, _tint_tmp[0], _tint_tmp[1]);\n" + "_tint_tmp[2] = _tint_tmp[1];", "_tint_tmp", }; case ValidTextureOverload::kSample1dF32: diff --git a/src/writer/msl/generator_impl.cc b/src/writer/msl/generator_impl.cc index 1769301a86..5343a11ffe 100644 --- a/src/writer/msl/generator_impl.cc +++ b/src/writer/msl/generator_impl.cc @@ -665,7 +665,37 @@ bool GeneratorImpl::EmitTextureCall(ast::CallExpression* expr) { auto& pidx = signature->params.idx; auto const kNotUsed = ast::intrinsic::TextureSignature::Parameters::kNotUsed; + assert(pidx.texture != kNotUsed); + auto* texture_type = params[pidx.texture] + ->result_type() + ->UnwrapAll() + ->As(); + if (ident->intrinsic() == ast::Intrinsic::kTextureDimensions) { + std::vector dims; + switch (texture_type->dim()) { + case ast::type::TextureDimension::kNone: + error_ = "texture dimension is kNone"; + return false; + case ast::type::TextureDimension::k1d: + case ast::type::TextureDimension::k1dArray: + dims = {"width"}; + break; + case ast::type::TextureDimension::k2d: + case ast::type::TextureDimension::k2dArray: + dims = {"width", "height"}; + break; + case ast::type::TextureDimension::k3d: + dims = {"width", "height", "depth"}; + break; + case ast::type::TextureDimension::kCube: + case ast::type::TextureDimension::kCubeArray: + // width == height == depth for cubes + // See https://github.com/gpuweb/gpuweb/issues/1345 + dims = {"width", "height", "height"}; + break; + } + auto get_dim = [&](const char* name) { if (!EmitExpression(params[pidx.texture])) { return false; @@ -678,32 +708,18 @@ bool GeneratorImpl::EmitTextureCall(ast::CallExpression* expr) { return true; }; - size_t dims = 1; - if (auto* vec = expr->result_type()->As()) { - dims = vec->size(); - } - switch (dims) { - case 1: - get_dim("width"); - break; - case 2: - EmitType(expr->result_type(), ""); - out_ << "("; - get_dim("width"); - out_ << ", "; - get_dim("height"); - out_ << ")"; - break; - case 3: - EmitType(expr->result_type(), ""); - out_ << "("; - get_dim("width"); - out_ << ", "; - get_dim("height"); - out_ << ", "; - get_dim("depth"); - out_ << ")"; - break; + if (dims.size() == 1) { + get_dim(dims[0]); + } else { + EmitType(expr->result_type(), ""); + out_ << "("; + for (size_t i = 0; i < dims.size(); i++) { + if (i > 0) { + out_ << ", "; + } + get_dim(dims[i]); + } + out_ << ")"; } return true; } diff --git a/src/writer/msl/generator_impl_intrinsic_texture_test.cc b/src/writer/msl/generator_impl_intrinsic_texture_test.cc index c7921d9b8e..fcc6bbe9f9 100644 --- a/src/writer/msl/generator_impl_intrinsic_texture_test.cc +++ b/src/writer/msl/generator_impl_intrinsic_texture_test.cc @@ -51,24 +51,26 @@ std::string expected_texture_overload( case ValidTextureOverload::kDimensionsStorageWO2dArray: return R"(int2(texture_tint_0.get_width(), texture_tint_0.get_height()))"; case ValidTextureOverload::kDimensions3d: + case ValidTextureOverload::kDimensionsStorageRO3d: + case ValidTextureOverload::kDimensionsStorageWO3d: + return R"(int3(texture_tint_0.get_width(), texture_tint_0.get_height(), texture_tint_0.get_depth()))"; case ValidTextureOverload::kDimensionsCube: case ValidTextureOverload::kDimensionsCubeArray: case ValidTextureOverload::kDimensionsDepthCube: case ValidTextureOverload::kDimensionsDepthCubeArray: - case ValidTextureOverload::kDimensionsStorageRO3d: - case ValidTextureOverload::kDimensionsStorageWO3d: - return R"(int3(texture_tint_0.get_width(), texture_tint_0.get_height(), texture_tint_0.get_depth()))"; + return R"(int3(texture_tint_0.get_width(), texture_tint_0.get_height(), texture_tint_0.get_height()))"; case ValidTextureOverload::kDimensions2dLevel: case ValidTextureOverload::kDimensions2dArrayLevel: case ValidTextureOverload::kDimensionsDepth2dLevel: case ValidTextureOverload::kDimensionsDepth2dArrayLevel: return R"(int2(texture_tint_0.get_width(1), texture_tint_0.get_height(1)))"; case ValidTextureOverload::kDimensions3dLevel: + return R"(int3(texture_tint_0.get_width(1), texture_tint_0.get_height(1), texture_tint_0.get_depth(1)))"; case ValidTextureOverload::kDimensionsCubeLevel: case ValidTextureOverload::kDimensionsCubeArrayLevel: case ValidTextureOverload::kDimensionsDepthCubeLevel: case ValidTextureOverload::kDimensionsDepthCubeArrayLevel: - return R"(int3(texture_tint_0.get_width(1), texture_tint_0.get_height(1), texture_tint_0.get_depth(1)))"; + return R"(int3(texture_tint_0.get_width(1), texture_tint_0.get_height(1), texture_tint_0.get_height(1)))"; case ValidTextureOverload::kSample1dF32: return R"(texture_tint_0.sample(sampler_tint_0, 1.0f))"; case ValidTextureOverload::kSample1dArrayF32: diff --git a/src/writer/spirv/builder.cc b/src/writer/spirv/builder.cc index bd6fda7931..68ca025ebd 100644 --- a/src/writer/spirv/builder.cc +++ b/src/writer/spirv/builder.cc @@ -272,6 +272,14 @@ uint32_t intrinsic_to_glsl_method(ast::type::Type* type, return 0; } +/// @return the vector element type if ty is a vector, otherwise return ty. +ast::type::Type* ElementTypeOf(ast::type::Type* ty) { + if (auto* v = ty->As()) { + return v->type(); + } + return ty; +} + } // namespace Builder::AccessorInfo::AccessorInfo() : source_id(0), source_type(nullptr) {} @@ -1987,9 +1995,6 @@ bool Builder::GenerateTextureIntrinsic(ast::IdentifierExpression* ident, ast::CallExpression* call, Operand result_type, Operand result_id) { - auto* texture_type = - call->params()[0]->result_type()->UnwrapAll()->As(); - auto* sig = static_cast( ident->intrinsic_signature()); assert(sig != nullptr); @@ -1997,6 +2002,10 @@ bool Builder::GenerateTextureIntrinsic(ast::IdentifierExpression* ident, auto const kNotUsed = ast::intrinsic::TextureSignature::Parameters::kNotUsed; assert(pidx.texture != kNotUsed); + auto* texture_type = call->params()[pidx.texture] + ->result_type() + ->UnwrapAll() + ->As(); auto op = spv::Op::OpNop; @@ -2070,50 +2079,76 @@ bool Builder::GenerateTextureIntrinsic(ast::IdentifierExpression* ident, switch (ident->intrinsic()) { case ast::Intrinsic::kTextureDimensions: { - if (ast::type::IsTextureArray(texture_type->dim())) { - // OpImageQuerySize[Lod] will append another element to the returned - // vector describing the number of array elements. textureDimensions() - // does not include this in the returned vector, so it needs to be - // stripped from the resulting vector. - auto unstripped_result = result_op(); + // Number of returned elements from OpImageQuerySize[Lod] may not match + // those of textureDimensions(). + // This might be due to an extra vector scalar describing the number of + // array elements or textureDimensions() returning a vec3 for cubes + // when only width / height is returned by OpImageQuerySize[Lod] + // (see https://github.com/gpuweb/gpuweb/issues/1345). + // Handle these mismatches by swizzling the returned vector. + std::vector swizzle; + uint32_t spirv_dims = 0; + switch (texture_type->dim()) { + case ast::type::TextureDimension::kNone: + error_ = "texture dimension is kNone"; + return false; + case ast::type::TextureDimension::k1d: + case ast::type::TextureDimension::k2d: + case ast::type::TextureDimension::k3d: + break; // No swizzle needed + case ast::type::TextureDimension::k1dArray: + swizzle = {0}; // Strip array index + spirv_dims = 2; // [width, array count] + break; + case ast::type::TextureDimension::kCube: + swizzle = {0, 1, 1}; // Duplicate height for depth + spirv_dims = 2; // [width, height] + break; + case ast::type::TextureDimension::k2dArray: + swizzle = {0, 1}; // Strip array index + spirv_dims = 3; // [width, height, array_count] + break; + case ast::type::TextureDimension::kCubeArray: + swizzle = {0, 1, 1}; // Strip array index, duplicate height for depth + spirv_dims = 3; // [width, height, array_count] + break; + } - ast::type::Type* unstripped_result_type; - if (auto* v = call->result_type()->As()) { - unstripped_result_type = - mod_->create(v->type(), v->size() + 1); + if (swizzle.empty()) { + append_result_type_and_id_to_spirv_params(); + } else { + // Assign post_emission to swizzle the result of the call to + // OpImageQuerySize[Lod]. + auto* element_type = ElementTypeOf(call->result_type()); + auto spirv_result = result_op(); + auto* spirv_result_type = + mod_->create(element_type, spirv_dims); + if (swizzle.size() > 1) { post_emission = [=] { - // Swizzle the unstripped vector to form a vec2 or vec3 OperandList operands{ result_type, result_id, - unstripped_result, - unstripped_result, + spirv_result, + spirv_result, }; - for (uint32_t i = 0; i < v->size(); i++) { - operands.emplace_back(Operand::Int(i)); + for (auto idx : swizzle) { + operands.emplace_back(Operand::Int(idx)); } return push_function_inst(spv::Op::OpVectorShuffle, operands); }; } else { - unstripped_result_type = - mod_->create(call->result_type(), 2); post_emission = [=] { - // Extract the first element of the unstripped vec2 to form a scalar - return push_function_inst( - spv::Op::OpCompositeExtract, - {result_type, result_id, unstripped_result, Operand::Int(0)}); + return push_function_inst(spv::Op::OpCompositeExtract, + {result_type, result_id, spirv_result, + Operand::Int(swizzle[0])}); }; } - - auto unstripped_result_type_id = - GenerateTypeIfNeeded(unstripped_result_type); - if (unstripped_result_type_id == 0) { + auto spirv_result_type_id = GenerateTypeIfNeeded(spirv_result_type); + if (spirv_result_type_id == 0) { return false; } - spirv_params.emplace_back(Operand::Int(unstripped_result_type_id)); - spirv_params.emplace_back(unstripped_result); - } else { - append_result_type_and_id_to_spirv_params(); + spirv_params.emplace_back(Operand::Int(spirv_result_type_id)); + spirv_params.emplace_back(spirv_result); } spirv_params.emplace_back(gen_param(pidx.texture)); diff --git a/src/writer/spirv/builder_intrinsic_texture_test.cc b/src/writer/spirv/builder_intrinsic_texture_test.cc index 760af94132..a86ccb2347 100644 --- a/src/writer/spirv/builder_intrinsic_texture_test.cc +++ b/src/writer/spirv/builder_intrinsic_texture_test.cc @@ -226,11 +226,13 @@ OpCapability ImageQuery %5 = OpVariable %6 UniformConstant %10 = OpTypeInt 32 1 %9 = OpTypeVector %10 3 -%12 = OpConstant %10 0 +%12 = OpTypeVector %10 2 +%14 = OpConstant %10 0 )", R"( -%11 = OpLoad %3 %1 -%8 = OpImageQuerySizeLod %9 %11 %12 +%13 = OpLoad %3 %1 +%11 = OpImageQuerySizeLod %12 %13 %14 +%8 = OpVectorShuffle %9 %11 %11 0 1 1 )", R"( OpCapability ImageQuery @@ -247,11 +249,13 @@ OpCapability ImageQuery %5 = OpVariable %6 UniformConstant %10 = OpTypeInt 32 1 %9 = OpTypeVector %10 3 -%12 = OpConstant %10 1 +%12 = OpTypeVector %10 2 +%14 = OpConstant %10 1 )", R"( -%11 = OpLoad %3 %1 -%8 = OpImageQuerySizeLod %9 %11 %12 +%13 = OpLoad %3 %1 +%11 = OpImageQuerySizeLod %12 %13 %14 +%8 = OpVectorShuffle %9 %11 %11 0 1 1 )", R"( OpCapability ImageQuery @@ -268,13 +272,12 @@ OpCapability ImageQuery %5 = OpVariable %6 UniformConstant %10 = OpTypeInt 32 1 %9 = OpTypeVector %10 3 -%12 = OpTypeVector %10 4 -%14 = OpConstant %10 0 +%13 = OpConstant %10 0 )", R"( -%13 = OpLoad %3 %1 -%11 = OpImageQuerySizeLod %12 %13 %14 -%8 = OpVectorShuffle %9 %11 %11 0 1 2 +%12 = OpLoad %3 %1 +%11 = OpImageQuerySizeLod %9 %12 %13 +%8 = OpVectorShuffle %9 %11 %11 0 1 1 )", R"( OpCapability SampledCubeArray @@ -292,13 +295,12 @@ OpCapability ImageQuery %5 = OpVariable %6 UniformConstant %10 = OpTypeInt 32 1 %9 = OpTypeVector %10 3 -%12 = OpTypeVector %10 4 -%14 = OpConstant %10 1 +%13 = OpConstant %10 1 )", R"( -%13 = OpLoad %3 %1 -%11 = OpImageQuerySizeLod %12 %13 %14 -%8 = OpVectorShuffle %9 %11 %11 0 1 2 +%12 = OpLoad %3 %1 +%11 = OpImageQuerySizeLod %9 %12 %13 +%8 = OpVectorShuffle %9 %11 %11 0 1 1 )", R"( OpCapability SampledCubeArray @@ -446,11 +448,13 @@ OpCapability ImageQuery %5 = OpVariable %6 UniformConstant %10 = OpTypeInt 32 1 %9 = OpTypeVector %10 3 -%12 = OpConstant %10 0 +%12 = OpTypeVector %10 2 +%14 = OpConstant %10 0 )", R"( -%11 = OpLoad %3 %1 -%8 = OpImageQuerySizeLod %9 %11 %12 +%13 = OpLoad %3 %1 +%11 = OpImageQuerySizeLod %12 %13 %14 +%8 = OpVectorShuffle %9 %11 %11 0 1 1 )", R"( OpCapability ImageQuery @@ -467,11 +471,13 @@ OpCapability ImageQuery %5 = OpVariable %6 UniformConstant %10 = OpTypeInt 32 1 %9 = OpTypeVector %10 3 -%12 = OpConstant %10 1 +%12 = OpTypeVector %10 2 +%14 = OpConstant %10 1 )", R"( -%11 = OpLoad %3 %1 -%8 = OpImageQuerySizeLod %9 %11 %12 +%13 = OpLoad %3 %1 +%11 = OpImageQuerySizeLod %12 %13 %14 +%8 = OpVectorShuffle %9 %11 %11 0 1 1 )", R"( OpCapability ImageQuery @@ -488,13 +494,12 @@ OpCapability ImageQuery %5 = OpVariable %6 UniformConstant %10 = OpTypeInt 32 1 %9 = OpTypeVector %10 3 -%12 = OpTypeVector %10 4 -%14 = OpConstant %10 0 +%13 = OpConstant %10 0 )", R"( -%13 = OpLoad %3 %1 -%11 = OpImageQuerySizeLod %12 %13 %14 -%8 = OpVectorShuffle %9 %11 %11 0 1 2 +%12 = OpLoad %3 %1 +%11 = OpImageQuerySizeLod %9 %12 %13 +%8 = OpVectorShuffle %9 %11 %11 0 1 1 )", R"( OpCapability SampledCubeArray @@ -512,13 +517,12 @@ OpCapability ImageQuery %5 = OpVariable %6 UniformConstant %10 = OpTypeInt 32 1 %9 = OpTypeVector %10 3 -%12 = OpTypeVector %10 4 -%14 = OpConstant %10 1 +%13 = OpConstant %10 1 )", R"( -%13 = OpLoad %3 %1 -%11 = OpImageQuerySizeLod %12 %13 %14 -%8 = OpVectorShuffle %9 %11 %11 0 1 2 +%12 = OpLoad %3 %1 +%11 = OpImageQuerySizeLod %9 %12 %13 +%8 = OpVectorShuffle %9 %11 %11 0 1 1 )", R"( OpCapability SampledCubeArray