Use Tint SingleEntryPoint transform in Vulkan/GL backends
Some Vulkan drivers don't handle multiple entrypoints well.
In addition, SPIRV-Cross translation can be wrong for
shader modules with multiple entrypoints. Always emit a single
SPIR-V entrypoint to workaround these issues.
This allows updating CopyTextureForBrowser to use a single
shader module, and it fixes some tests with multiple
entrypoints.
Fixed: dawn:948, dawn:959, dawn:1013
Change-Id: Ie129a32a54845316d11917331937ca44fba3d347
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/60640
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Ben Clayton <bclayton@google.com>
diff --git a/src/dawn_native/CopyTextureForBrowserHelper.cpp b/src/dawn_native/CopyTextureForBrowserHelper.cpp
index 46da2c8..d64870c 100644
--- a/src/dawn_native/CopyTextureForBrowserHelper.cpp
+++ b/src/dawn_native/CopyTextureForBrowserHelper.cpp
@@ -33,17 +33,14 @@
namespace dawn_native {
namespace {
-// TODO(crbug.com/1221110): Remove this header macro by merging vertex and
-// fragment shaders into one shader source. Now it blocks by
-// crbug.com/dawn/947 and crbug.com/tint/915
-#define HEADER \
- " [[block]] struct Uniforms {\n" \
- " u_scale: vec2<f32>;\n" \
- " u_offset: vec2<f32>;\n" \
- " u_alphaOp: u32;\n" \
- " };\n"
- static const char sCopyTextureForBrowserVertex[] = HEADER R"(
+ static const char sCopyTextureForBrowserShader[] = R"(
+ [[block]] struct Uniforms {
+ u_scale: vec2<f32>;
+ u_offset: vec2<f32>;
+ u_alphaOp: u32;
+ };
+
[[binding(0), group(0)]] var<uniform> uniforms : Uniforms;
struct VertexOutputs {
@@ -51,7 +48,7 @@
[[builtin(position)]] position : vec4<f32>;
};
- [[stage(vertex)]] fn main(
+ [[stage(vertex)]] fn vs_main(
[[builtin(vertex_index)]] VertexIndex : u32
) -> VertexOutputs {
var texcoord = array<vec2<f32>, 3>(
@@ -84,14 +81,11 @@
return output;
}
- )";
- static const char sCopyTextureForBrowserFragment[] = HEADER R"(
- [[binding(0), group(0)]] var<uniform> uniforms : Uniforms;
[[binding(1), group(0)]] var mySampler: sampler;
[[binding(2), group(0)]] var myTexture: texture_2d<f32>;
- [[stage(fragment)]] fn main(
+ [[stage(fragment)]] fn fs_main(
[[location(0)]] texcoord : vec2<f32>
) -> [[location(0)]] vec4<f32> {
// Clamp the texcoord and discard the out-of-bound pixels.
@@ -226,39 +220,27 @@
if (GetCachedPipeline(store, dstFormat) == nullptr) {
// Create vertex shader module if not cached before.
- if (store->copyTextureForBrowserVS == nullptr) {
+ if (store->copyTextureForBrowser == nullptr) {
ShaderModuleDescriptor descriptor;
ShaderModuleWGSLDescriptor wgslDesc;
- wgslDesc.source = sCopyTextureForBrowserVertex;
+ wgslDesc.source = sCopyTextureForBrowserShader;
descriptor.nextInChain = reinterpret_cast<ChainedStruct*>(&wgslDesc);
- DAWN_TRY_ASSIGN(store->copyTextureForBrowserVS,
+ DAWN_TRY_ASSIGN(store->copyTextureForBrowser,
device->CreateShaderModule(&descriptor));
}
- ShaderModuleBase* vertexModule = store->copyTextureForBrowserVS.Get();
-
- // Create fragment shader module if not cached before.
- if (store->copyTextureForBrowserFS == nullptr) {
- ShaderModuleDescriptor descriptor;
- ShaderModuleWGSLDescriptor wgslDesc;
- wgslDesc.source = sCopyTextureForBrowserFragment;
- descriptor.nextInChain = reinterpret_cast<ChainedStruct*>(&wgslDesc);
- DAWN_TRY_ASSIGN(store->copyTextureForBrowserFS,
- device->CreateShaderModule(&descriptor));
- }
-
- ShaderModuleBase* fragmentModule = store->copyTextureForBrowserFS.Get();
+ ShaderModuleBase* shaderModule = store->copyTextureForBrowser.Get();
// Prepare vertex stage.
VertexState vertex = {};
- vertex.module = vertexModule;
- vertex.entryPoint = "main";
+ vertex.module = shaderModule;
+ vertex.entryPoint = "vs_main";
// Prepare frgament stage.
FragmentState fragment = {};
- fragment.module = fragmentModule;
- fragment.entryPoint = "main";
+ fragment.module = shaderModule;
+ fragment.entryPoint = "fs_main";
// Prepare color state.
ColorTargetState target = {};
diff --git a/src/dawn_native/InternalPipelineStore.h b/src/dawn_native/InternalPipelineStore.h
index 1d90159..3066a9a 100644
--- a/src/dawn_native/InternalPipelineStore.h
+++ b/src/dawn_native/InternalPipelineStore.h
@@ -28,8 +28,7 @@
std::unordered_map<wgpu::TextureFormat, Ref<RenderPipelineBase>>
copyTextureForBrowserPipelines;
- Ref<ShaderModuleBase> copyTextureForBrowserVS;
- Ref<ShaderModuleBase> copyTextureForBrowserFS;
+ Ref<ShaderModuleBase> copyTextureForBrowser;
Ref<ComputePipelineBase> timestampComputePipeline;
Ref<ShaderModuleBase> timestampCS;
diff --git a/src/dawn_native/opengl/ComputePipelineGL.cpp b/src/dawn_native/opengl/ComputePipelineGL.cpp
index 837b928..7680cee 100644
--- a/src/dawn_native/opengl/ComputePipelineGL.cpp
+++ b/src/dawn_native/opengl/ComputePipelineGL.cpp
@@ -18,12 +18,19 @@
namespace dawn_native { namespace opengl {
- ComputePipeline::ComputePipeline(Device* device, const ComputePipelineDescriptor* descriptor)
- : ComputePipelineBase(device, descriptor) {
- PerStage<const ShaderModule*> modules(nullptr);
- modules[SingleShaderStage::Compute] = ToBackend(descriptor->compute.module);
+ // static
+ ResultOrError<Ref<ComputePipeline>> ComputePipeline::Create(
+ Device* device,
+ const ComputePipelineDescriptor* descriptor) {
+ Ref<ComputePipeline> pipeline = AcquireRef(new ComputePipeline(device, descriptor));
+ DAWN_TRY(pipeline->Initialize(descriptor));
+ return pipeline;
+ }
- PipelineGL::Initialize(device->gl, ToBackend(descriptor->layout), GetAllStages());
+ MaybeError ComputePipeline::Initialize(const ComputePipelineDescriptor*) {
+ DAWN_TRY(
+ InitializeBase(ToBackend(GetDevice())->gl, ToBackend(GetLayout()), GetAllStages()));
+ return {};
}
void ComputePipeline::ApplyNow() {
diff --git a/src/dawn_native/opengl/ComputePipelineGL.h b/src/dawn_native/opengl/ComputePipelineGL.h
index 95f6db9..e84e366 100644
--- a/src/dawn_native/opengl/ComputePipelineGL.h
+++ b/src/dawn_native/opengl/ComputePipelineGL.h
@@ -27,12 +27,16 @@
class ComputePipeline final : public ComputePipelineBase, public PipelineGL {
public:
- ComputePipeline(Device* device, const ComputePipelineDescriptor* descriptor);
+ static ResultOrError<Ref<ComputePipeline>> Create(
+ Device* device,
+ const ComputePipelineDescriptor* descriptor);
void ApplyNow();
private:
+ using ComputePipelineBase::ComputePipelineBase;
~ComputePipeline() override = default;
+ MaybeError Initialize(const ComputePipelineDescriptor* descriptor) override;
};
}} // namespace dawn_native::opengl
diff --git a/src/dawn_native/opengl/DeviceGL.cpp b/src/dawn_native/opengl/DeviceGL.cpp
index dd38faa..fb7b4ea 100644
--- a/src/dawn_native/opengl/DeviceGL.cpp
+++ b/src/dawn_native/opengl/DeviceGL.cpp
@@ -129,7 +129,7 @@
}
ResultOrError<Ref<ComputePipelineBase>> Device::CreateComputePipelineImpl(
const ComputePipelineDescriptor* descriptor) {
- return AcquireRef(new ComputePipeline(this, descriptor));
+ return ComputePipeline::Create(this, descriptor);
}
ResultOrError<Ref<PipelineLayoutBase>> Device::CreatePipelineLayoutImpl(
const PipelineLayoutDescriptor* descriptor) {
@@ -141,7 +141,7 @@
}
ResultOrError<Ref<RenderPipelineBase>> Device::CreateRenderPipelineImpl(
const RenderPipelineDescriptor* descriptor) {
- return AcquireRef(new RenderPipeline(this, descriptor));
+ return RenderPipeline::Create(this, descriptor);
}
ResultOrError<Ref<SamplerBase>> Device::CreateSamplerImpl(const SamplerDescriptor* descriptor) {
return AcquireRef(new Sampler(this, descriptor));
diff --git a/src/dawn_native/opengl/PipelineGL.cpp b/src/dawn_native/opengl/PipelineGL.cpp
index 0110025..4541e65 100644
--- a/src/dawn_native/opengl/PipelineGL.cpp
+++ b/src/dawn_native/opengl/PipelineGL.cpp
@@ -15,7 +15,6 @@
#include "dawn_native/opengl/PipelineGL.h"
#include "common/BitSetIterator.h"
-#include "common/Log.h"
#include "dawn_native/BindGroupLayout.h"
#include "dawn_native/Device.h"
#include "dawn_native/Pipeline.h"
@@ -26,6 +25,7 @@
#include "dawn_native/opengl/ShaderModuleGL.h"
#include <set>
+#include <sstream>
namespace dawn_native { namespace opengl {
@@ -47,11 +47,11 @@
PipelineGL::PipelineGL() = default;
PipelineGL::~PipelineGL() = default;
- void PipelineGL::Initialize(const OpenGLFunctions& gl,
- const PipelineLayout* layout,
- const PerStage<ProgrammableStage>& stages) {
+ MaybeError PipelineGL::InitializeBase(const OpenGLFunctions& gl,
+ const PipelineLayout* layout,
+ const PerStage<ProgrammableStage>& stages) {
auto CreateShader = [](const OpenGLFunctions& gl, GLenum type,
- const char* source) -> GLuint {
+ const char* source) -> ResultOrError<GLuint> {
GLuint shader = gl.CreateShader(type);
gl.ShaderSource(shader, 1, &source, nullptr);
gl.CompileShader(shader);
@@ -65,8 +65,9 @@
if (infoLogLength > 1) {
std::vector<char> buffer(infoLogLength);
gl.GetShaderInfoLog(shader, infoLogLength, nullptr, &buffer[0]);
- dawn::ErrorLog() << source << "\nProgram compilation failed:\n"
- << buffer.data();
+ std::stringstream ss;
+ ss << source << "\nProgram compilation failed:\n" << buffer.data();
+ return DAWN_VALIDATION_ERROR(ss.str().c_str());
}
}
return shader;
@@ -87,10 +88,12 @@
bool needsDummySampler = false;
for (SingleShaderStage stage : IterateStages(activeStages)) {
const ShaderModule* module = ToBackend(stages[stage].module.Get());
- std::string glsl =
- module->TranslateToGLSL(stages[stage].entryPoint.c_str(), stage,
- &combinedSamplers[stage], layout, &needsDummySampler);
- GLuint shader = CreateShader(gl, GLShaderType(stage), glsl.c_str());
+ std::string glsl;
+ DAWN_TRY_ASSIGN(glsl, module->TranslateToGLSL(stages[stage].entryPoint.c_str(), stage,
+ &combinedSamplers[stage], layout,
+ &needsDummySampler));
+ GLuint shader;
+ DAWN_TRY_ASSIGN(shader, CreateShader(gl, GLShaderType(stage), glsl.c_str()));
gl.AttachShader(mProgram, shader);
}
@@ -115,7 +118,9 @@
if (infoLogLength > 1) {
std::vector<char> buffer(infoLogLength);
gl.GetProgramInfoLog(mProgram, infoLogLength, nullptr, &buffer[0]);
- dawn::ErrorLog() << "Program link failed:\n" << buffer.data();
+ std::stringstream ss;
+ ss << "Program link failed:\n" << buffer.data();
+ return DAWN_VALIDATION_ERROR(ss.str().c_str());
}
}
@@ -172,6 +177,7 @@
textureUnit++;
}
+ return {};
}
const std::vector<PipelineGL::SamplerUnit>& PipelineGL::GetTextureUnitsForSampler(
diff --git a/src/dawn_native/opengl/PipelineGL.h b/src/dawn_native/opengl/PipelineGL.h
index 33e5341..e210606 100644
--- a/src/dawn_native/opengl/PipelineGL.h
+++ b/src/dawn_native/opengl/PipelineGL.h
@@ -37,10 +37,6 @@
PipelineGL();
~PipelineGL();
- void Initialize(const OpenGLFunctions& gl,
- const PipelineLayout* layout,
- const PerStage<ProgrammableStage>& stages);
-
// For each unit a sampler is bound to we need to know if we should use filtering or not
// because int and uint texture are only complete without filtering.
struct SamplerUnit {
@@ -53,6 +49,11 @@
void ApplyNow(const OpenGLFunctions& gl);
+ protected:
+ MaybeError InitializeBase(const OpenGLFunctions& gl,
+ const PipelineLayout* layout,
+ const PerStage<ProgrammableStage>& stages);
+
private:
GLuint mProgram;
std::vector<std::vector<SamplerUnit>> mUnitsForSamplers;
diff --git a/src/dawn_native/opengl/RenderPipelineGL.cpp b/src/dawn_native/opengl/RenderPipelineGL.cpp
index aed8c01..5bc596f 100644
--- a/src/dawn_native/opengl/RenderPipelineGL.cpp
+++ b/src/dawn_native/opengl/RenderPipelineGL.cpp
@@ -219,16 +219,26 @@
} // anonymous namespace
+ // static
+ ResultOrError<Ref<RenderPipeline>> RenderPipeline::Create(
+ Device* device,
+ const RenderPipelineDescriptor* descriptor) {
+ Ref<RenderPipeline> pipeline = AcquireRef(new RenderPipeline(device, descriptor));
+ DAWN_TRY(pipeline->Initialize());
+ return pipeline;
+ }
+
RenderPipeline::RenderPipeline(Device* device, const RenderPipelineDescriptor* descriptor)
: RenderPipelineBase(device, descriptor),
mVertexArrayObject(0),
mGlPrimitiveTopology(GLPrimitiveTopology(GetPrimitiveTopology())) {
- PerStage<const ShaderModule*> modules(nullptr);
- modules[SingleShaderStage::Vertex] = ToBackend(descriptor->vertex.module);
- modules[SingleShaderStage::Fragment] = ToBackend(descriptor->fragment->module);
+ }
- PipelineGL::Initialize(device->gl, ToBackend(GetLayout()), GetAllStages());
+ MaybeError RenderPipeline::Initialize() {
+ DAWN_TRY(
+ InitializeBase(ToBackend(GetDevice())->gl, ToBackend(GetLayout()), GetAllStages()));
CreateVAOForVertexState();
+ return {};
}
RenderPipeline::~RenderPipeline() {
diff --git a/src/dawn_native/opengl/RenderPipelineGL.h b/src/dawn_native/opengl/RenderPipelineGL.h
index 960b50b..3c7a4d3 100644
--- a/src/dawn_native/opengl/RenderPipelineGL.h
+++ b/src/dawn_native/opengl/RenderPipelineGL.h
@@ -29,7 +29,9 @@
class RenderPipeline final : public RenderPipelineBase, public PipelineGL {
public:
- RenderPipeline(Device* device, const RenderPipelineDescriptor* descriptor);
+ static ResultOrError<Ref<RenderPipeline>> Create(
+ Device* device,
+ const RenderPipelineDescriptor* descriptor);
GLenum GetGLPrimitiveTopology() const;
ityp::bitset<VertexAttributeLocation, kMaxVertexAttributes> GetAttributesUsingVertexBuffer(
@@ -38,7 +40,10 @@
void ApplyNow(PersistentPipelineState& persistentPipelineState);
private:
+ RenderPipeline(Device* device, const RenderPipelineDescriptor* descriptor);
~RenderPipeline() override;
+ MaybeError Initialize();
+
void CreateVAOForVertexState();
// TODO(yunchao.he@intel.com): vao need to be deduplicated between pipelines.
diff --git a/src/dawn_native/opengl/ShaderModuleGL.cpp b/src/dawn_native/opengl/ShaderModuleGL.cpp
index a5bbcb9..6aac35de 100644
--- a/src/dawn_native/opengl/ShaderModuleGL.cpp
+++ b/src/dawn_native/opengl/ShaderModuleGL.cpp
@@ -94,18 +94,18 @@
return DAWN_VALIDATION_ERROR(errorStream.str().c_str());
}
- mGLSpirv = std::move(result.spirv);
- DAWN_TRY_ASSIGN(mGLEntryPoints, ReflectShaderUsingSPIRVCross(GetDevice(), mGLSpirv));
+ DAWN_TRY_ASSIGN(mGLEntryPoints,
+ ReflectShaderUsingSPIRVCross(GetDevice(), result.spirv));
}
return {};
}
- std::string ShaderModule::TranslateToGLSL(const char* entryPointName,
- SingleShaderStage stage,
- CombinedSamplerInfo* combinedSamplers,
- const PipelineLayout* layout,
- bool* needsDummySampler) const {
+ ResultOrError<std::string> ShaderModule::TranslateToGLSL(const char* entryPointName,
+ SingleShaderStage stage,
+ CombinedSamplerInfo* combinedSamplers,
+ const PipelineLayout* layout,
+ bool* needsDummySampler) const {
// If these options are changed, the values in DawnSPIRVCrossGLSLFastFuzzer.cpp need to
// be updated.
spirv_cross::CompilerGLSL::Options options;
@@ -125,8 +125,32 @@
options.es = version.IsES();
options.version = version.GetMajor() * 100 + version.GetMinor() * 10;
- spirv_cross::CompilerGLSL compiler(
- GetDevice()->IsToggleEnabled(Toggle::UseTintGenerator) ? mGLSpirv : GetSpirv());
+ std::vector<uint32_t> spirv;
+ if (GetDevice()->IsToggleEnabled(Toggle::UseTintGenerator)) {
+ tint::transform::SingleEntryPoint singleEntryPointTransform;
+
+ tint::transform::DataMap transformInputs;
+ transformInputs.Add<tint::transform::SingleEntryPoint::Config>(entryPointName);
+
+ tint::Program program;
+ DAWN_TRY_ASSIGN(program, RunTransforms(&singleEntryPointTransform, GetTintProgram(),
+ transformInputs, nullptr, nullptr));
+
+ tint::writer::spirv::Options options;
+ options.disable_workgroup_init =
+ GetDevice()->IsToggleEnabled(Toggle::DisableWorkgroupInit);
+ auto result = tint::writer::spirv::Generate(&program, options);
+ if (!result.success) {
+ std::ostringstream errorStream;
+ errorStream << "Generator: " << result.error << std::endl;
+ return DAWN_VALIDATION_ERROR(errorStream.str().c_str());
+ }
+
+ spirv = std::move(result.spirv);
+ } else {
+ spirv = GetSpirv();
+ }
+ spirv_cross::CompilerGLSL compiler(std::move(spirv));
compiler.set_common_options(options);
compiler.set_entry_point(entryPointName, ShaderStageToExecutionModel(stage));
diff --git a/src/dawn_native/opengl/ShaderModuleGL.h b/src/dawn_native/opengl/ShaderModuleGL.h
index 7f598b9..d6d9f74 100644
--- a/src/dawn_native/opengl/ShaderModuleGL.h
+++ b/src/dawn_native/opengl/ShaderModuleGL.h
@@ -50,18 +50,17 @@
const ShaderModuleDescriptor* descriptor,
ShaderModuleParseResult* parseResult);
- std::string TranslateToGLSL(const char* entryPointName,
- SingleShaderStage stage,
- CombinedSamplerInfo* combinedSamplers,
- const PipelineLayout* layout,
- bool* needsDummySampler) const;
+ ResultOrError<std::string> TranslateToGLSL(const char* entryPointName,
+ SingleShaderStage stage,
+ CombinedSamplerInfo* combinedSamplers,
+ const PipelineLayout* layout,
+ bool* needsDummySampler) const;
private:
ShaderModule(Device* device, const ShaderModuleDescriptor* descriptor);
~ShaderModule() override = default;
MaybeError Initialize(ShaderModuleParseResult* parseResult);
- std::vector<uint32_t> mGLSpirv;
EntryPointMetadataTable mGLEntryPoints;
};
diff --git a/src/dawn_native/vulkan/ShaderModuleVk.cpp b/src/dawn_native/vulkan/ShaderModuleVk.cpp
index 525dbb1..0144eef 100644
--- a/src/dawn_native/vulkan/ShaderModuleVk.cpp
+++ b/src/dawn_native/vulkan/ShaderModuleVk.cpp
@@ -104,7 +104,6 @@
DAWN_TRY_ASSIGN(program,
RunTransforms(&transformManager, parseResult->tintProgram.get(),
transformInputs, nullptr, nullptr));
- // We will miss the messages generated in this RunTransforms.
tint::writer::spirv::Options options;
options.emit_vertex_point_size = true;
@@ -203,11 +202,14 @@
tint::transform::Manager transformManager;
transformManager.append(std::make_unique<tint::transform::BindingRemapper>());
+ // Many Vulkan drivers can't handle multi-entrypoint shader modules.
+ transformManager.append(std::make_unique<tint::transform::SingleEntryPoint>());
tint::transform::DataMap transformInputs;
transformInputs.Add<BindingRemapper::Remappings>(std::move(bindingPoints),
std::move(accessControls),
/* mayCollide */ false);
+ transformInputs.Add<tint::transform::SingleEntryPoint::Config>(entryPointName);
tint::Program program;
DAWN_TRY_ASSIGN(program, RunTransforms(&transformManager, GetTintProgram(), transformInputs,
diff --git a/src/tests/end2end/CopyTextureForBrowserTests.cpp b/src/tests/end2end/CopyTextureForBrowserTests.cpp
index 1aca79d..768e36a 100644
--- a/src/tests/end2end/CopyTextureForBrowserTests.cpp
+++ b/src/tests/end2end/CopyTextureForBrowserTests.cpp
@@ -140,6 +140,10 @@
void SetUp() override {
DawnTest::SetUp();
+ // crbug.com/dawn/948: Tint required for multiple entrypoints in a module.
+ // CopyTextureForBrowser uses and internal pipeline with a multi-entrypoint
+ // shader module.
+ DAWN_TEST_UNSUPPORTED_IF(!HasToggleEnabled("use_tint_generator"));
testPipeline = MakeTestPipeline();
diff --git a/src/tests/end2end/ShaderTests.cpp b/src/tests/end2end/ShaderTests.cpp
index fa427aa..c635048 100644
--- a/src/tests/end2end/ShaderTests.cpp
+++ b/src/tests/end2end/ShaderTests.cpp
@@ -262,8 +262,8 @@
// Tests that shaders I/O structs can be shared between vertex and fragment shaders.
TEST_P(ShaderTests, WGSLSharedStructIO) {
- // TODO(tint:714): Not yet implemeneted in tint yet, but intended to work.
- DAWN_SUPPRESS_TEST_IF(IsD3D12() || IsVulkan() || IsMetal() || IsOpenGL() || IsOpenGLES());
+ // crbug.com/dawn/948: Tint required for multiple entrypoints in a module.
+ DAWN_TEST_UNSUPPORTED_IF(!HasToggleEnabled("use_tint_generator"));
std::string shader = R"(
struct VertexIn {
