From b27e2bfa71fec4320400351c44835a6192ce0962 Mon Sep 17 00:00:00 2001 From: Loko Kung Date: Wed, 3 May 2023 02:23:25 +0000 Subject: [PATCH] Adds generation for WGSL descriptor to rename source->code. - Adds a deprecation warning for the incorrect usage. - Adds test to verify that the warning is emitted and that the results are equal. - Fixes all existing tests to use the newer code path. Change-Id: I01bf85137ad1c66966e1aa0259ae03fc5247ba25 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/130320 Kokoro: Kokoro Commit-Queue: Loko Kung Reviewed-by: Austin Eng --- dawn.json | 4 +-- generator/templates/dawn/wire/WireCmd.cpp | 2 ++ src/dawn/native/BlitBufferToDepthStencil.cpp | 4 +-- src/dawn/native/BlitDepthToDepth.cpp | 2 +- src/dawn/native/Device.cpp | 2 +- src/dawn/native/ShaderModule.cpp | 20 ++++++++--- src/dawn/native/utils/WGPUHelpers.cpp | 2 +- .../unittests/native/DestroyObjectTests.cpp | 6 ++-- .../native/mocks/ShaderModuleMock.cpp | 2 +- .../tests/unittests/validation/LabelTests.cpp | 2 +- .../validation/MultipleDeviceTests.cpp | 2 +- .../ShaderModuleValidationTests.cpp | 36 +++++++++++++++++-- .../wire/WireDeviceLifetimeTests.cpp | 2 +- src/dawn/utils/WGPUHelpers.cpp | 2 +- 14 files changed, 67 insertions(+), 21 deletions(-) diff --git a/dawn.json b/dawn.json index dad6e4e297..d4a86caad7 100644 --- a/dawn.json +++ b/dawn.json @@ -2408,8 +2408,8 @@ "chained": "in", "chain roots": ["shader module descriptor"], "members": [ - {"name": "source", "type": "char", "annotation": "const*", "length": "strlen", "tags": ["dawn", "emscripten"]}, - {"name": "code", "type": "char", "annotation": "const*", "length": "strlen", "tags": ["upstream"]} + {"name": "source", "type": "char", "annotation": "const*", "length": "strlen", "default": "nullptr", "optional": true, "tags": ["dawn", "emscripten"]}, + {"name": "code", "type": "char", "annotation": "const*", "length": "strlen", "default": "nullptr", "optional": true} ] }, "dawn shader module SPIRV options descriptor": { diff --git a/generator/templates/dawn/wire/WireCmd.cpp b/generator/templates/dawn/wire/WireCmd.cpp index 10a7d36e31..ebf15d2c27 100644 --- a/generator/templates/dawn/wire/WireCmd.cpp +++ b/generator/templates/dawn/wire/WireCmd.cpp @@ -163,6 +163,8 @@ {% if member.optional %} bool has_{{memberName}} = record.{{memberName}} != nullptr; if (has_{{memberName}}) + {% else %} + ASSERT(record.{{memberName}} != nullptr); {% endif %} { result += Align(std::strlen(record.{{memberName}}), kWireBufferAlignment); diff --git a/src/dawn/native/BlitBufferToDepthStencil.cpp b/src/dawn/native/BlitBufferToDepthStencil.cpp index f4db65e73d..879d21a374 100644 --- a/src/dawn/native/BlitBufferToDepthStencil.cpp +++ b/src/dawn/native/BlitBufferToDepthStencil.cpp @@ -125,7 +125,7 @@ ResultOrError> GetOrCreateRG8ToDepth16UnormPipeline(Devi ShaderModuleWGSLDescriptor wgslDesc = {}; ShaderModuleDescriptor shaderModuleDesc = {}; shaderModuleDesc.nextInChain = &wgslDesc; - wgslDesc.source = kBlitRG8ToDepthShaders; + wgslDesc.code = kBlitRG8ToDepthShaders; Ref shaderModule; DAWN_TRY_ASSIGN(shaderModule, device->CreateShaderModule(&shaderModuleDesc)); @@ -176,7 +176,7 @@ ResultOrError GetOrCreateR8ToSt ShaderModuleWGSLDescriptor wgslDesc = {}; ShaderModuleDescriptor shaderModuleDesc = {}; shaderModuleDesc.nextInChain = &wgslDesc; - wgslDesc.source = kBlitStencilShaders; + wgslDesc.code = kBlitStencilShaders; Ref shaderModule; DAWN_TRY_ASSIGN(shaderModule, device->CreateShaderModule(&shaderModuleDesc)); diff --git a/src/dawn/native/BlitDepthToDepth.cpp b/src/dawn/native/BlitDepthToDepth.cpp index 82930995ce..f26ddd9ed4 100644 --- a/src/dawn/native/BlitDepthToDepth.cpp +++ b/src/dawn/native/BlitDepthToDepth.cpp @@ -63,7 +63,7 @@ ResultOrError> GetOrCreateDepthBlitPipeline(DeviceBase* ShaderModuleWGSLDescriptor wgslDesc = {}; ShaderModuleDescriptor shaderModuleDesc = {}; shaderModuleDesc.nextInChain = &wgslDesc; - wgslDesc.source = kBlitToDepthShaders; + wgslDesc.code = kBlitToDepthShaders; Ref shaderModule; DAWN_TRY_ASSIGN(shaderModule, device->CreateShaderModule(&shaderModuleDesc)); diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp index 4a79454b22..d3a87d4361 100644 --- a/src/dawn/native/Device.cpp +++ b/src/dawn/native/Device.cpp @@ -270,7 +270,7 @@ MaybeError DeviceBase::Initialize(Ref defaultQueue) { )"; ShaderModuleDescriptor descriptor; ShaderModuleWGSLDescriptor wgslDesc; - wgslDesc.source = kEmptyFragmentShader; + wgslDesc.code = kEmptyFragmentShader; descriptor.nextInChain = &wgslDesc; DAWN_TRY_ASSIGN(mInternalPipelineStore->placeholderFragmentShader, diff --git a/src/dawn/native/ShaderModule.cpp b/src/dawn/native/ShaderModule.cpp index 32b68ad3d2..69bedd2f6a 100644 --- a/src/dawn/native/ShaderModule.cpp +++ b/src/dawn/native/ShaderModule.cpp @@ -955,7 +955,7 @@ MaybeError ValidateAndParseShaderModule(DeviceBase* device, DAWN_INVALID_IF(!result.success, "Tint WGSL failure: Generator: %s", result.error); newWgslCode = std::move(result.wgsl); - newWgslDesc.source = newWgslCode.c_str(); + newWgslDesc.code = newWgslCode.c_str(); spirvDesc = nullptr; wgslDesc = &newWgslDesc; @@ -980,11 +980,16 @@ MaybeError ValidateAndParseShaderModule(DeviceBase* device, ASSERT(wgslDesc != nullptr); - auto tintSource = std::make_unique("", wgslDesc->source); + const char* code = wgslDesc->code ? wgslDesc->code : wgslDesc->source; + DAWN_INVALID_IF(code == nullptr, + "At least one of ShaderModuleWGSLDescriptor.source or " + "ShaderModuleWGSLDescriptor.code must be set."); + + auto tintSource = std::make_unique("", code); if (device->IsToggleEnabled(Toggle::DumpShaders)) { std::ostringstream dumpedMsg; - dumpedMsg << "// Dumped WGSL:" << std::endl << wgslDesc->source; + dumpedMsg << "// Dumped WGSL:" << std::endl << code; device->EmitLog(WGPULoggingType_Info, dumpedMsg.str().c_str()); } @@ -1101,7 +1106,14 @@ ShaderModuleBase::ShaderModuleBase(DeviceBase* device, mOriginalSpirv.assign(spirvDesc->code, spirvDesc->code + spirvDesc->codeSize); } else if (wgslDesc) { mType = Type::Wgsl; - mWgsl = std::string(wgslDesc->source); + if (wgslDesc->code) { + mWgsl = std::string(wgslDesc->code); + } else { + device->EmitDeprecationWarning( + "ShaderModuleWGSLDescriptor.source is deprecated, use " + "ShaderModuleWGSLDescriptor.code instead."); + mWgsl = std::string(wgslDesc->source); + } } } diff --git a/src/dawn/native/utils/WGPUHelpers.cpp b/src/dawn/native/utils/WGPUHelpers.cpp index deee395af7..3913a3999d 100644 --- a/src/dawn/native/utils/WGPUHelpers.cpp +++ b/src/dawn/native/utils/WGPUHelpers.cpp @@ -36,7 +36,7 @@ namespace dawn::native::utils { ResultOrError> CreateShaderModule(DeviceBase* device, const char* source) { ShaderModuleWGSLDescriptor wgslDesc; - wgslDesc.source = source; + wgslDesc.code = source; ShaderModuleDescriptor descriptor; descriptor.nextInChain = &wgslDesc; return device->CreateShaderModule(&descriptor); diff --git a/src/dawn/tests/unittests/native/DestroyObjectTests.cpp b/src/dawn/tests/unittests/native/DestroyObjectTests.cpp index 8151f795fb..d2af77566b 100644 --- a/src/dawn/tests/unittests/native/DestroyObjectTests.cpp +++ b/src/dawn/tests/unittests/native/DestroyObjectTests.cpp @@ -618,7 +618,7 @@ TEST_F(DestroyObjectTests, ShaderModuleNativeExplicit) { TEST_F(DestroyObjectTests, ShaderModuleImplicit) { ShaderModuleWGSLDescriptor wgslDesc = {}; - wgslDesc.source = kVertexShader.data(); + wgslDesc.code = kVertexShader.data(); ShaderModuleDescriptor desc = {}; desc.nextInChain = &wgslDesc; @@ -816,7 +816,7 @@ TEST_F(DestroyObjectTests, DestroyObjectsApiExplicit) { wgpu::ShaderModule csModule; { ShaderModuleWGSLDescriptor wgslDesc = {}; - wgslDesc.source = kComputeShader.data(); + wgslDesc.code = kComputeShader.data(); ShaderModuleDescriptor desc = {}; desc.nextInChain = &wgslDesc; @@ -830,7 +830,7 @@ TEST_F(DestroyObjectTests, DestroyObjectsApiExplicit) { wgpu::ShaderModule vsModule; { ShaderModuleWGSLDescriptor wgslDesc = {}; - wgslDesc.source = kVertexShader.data(); + wgslDesc.code = kVertexShader.data(); ShaderModuleDescriptor desc = {}; desc.nextInChain = &wgslDesc; diff --git a/src/dawn/tests/unittests/native/mocks/ShaderModuleMock.cpp b/src/dawn/tests/unittests/native/mocks/ShaderModuleMock.cpp index e21add2c03..8b19df8bba 100644 --- a/src/dawn/tests/unittests/native/mocks/ShaderModuleMock.cpp +++ b/src/dawn/tests/unittests/native/mocks/ShaderModuleMock.cpp @@ -43,7 +43,7 @@ Ref ShaderModuleMock::Create(DeviceMock* device, // static Ref ShaderModuleMock::Create(DeviceMock* device, const char* source) { ShaderModuleWGSLDescriptor wgslDesc = {}; - wgslDesc.source = source; + wgslDesc.code = source; ShaderModuleDescriptor desc = {}; desc.nextInChain = &wgslDesc; return Create(device, &desc); diff --git a/src/dawn/tests/unittests/validation/LabelTests.cpp b/src/dawn/tests/unittests/validation/LabelTests.cpp index 82e7a35354..b99a13f741 100644 --- a/src/dawn/tests/unittests/validation/LabelTests.cpp +++ b/src/dawn/tests/unittests/validation/LabelTests.cpp @@ -615,7 +615,7 @@ TEST_F(LabelTest, ShaderModule) { })"; wgpu::ShaderModuleWGSLDescriptor wgslDesc; - wgslDesc.source = source; + wgslDesc.code = source; wgpu::ShaderModuleDescriptor descriptor; descriptor.nextInChain = &wgslDesc; diff --git a/src/dawn/tests/unittests/validation/MultipleDeviceTests.cpp b/src/dawn/tests/unittests/validation/MultipleDeviceTests.cpp index cb12a477b1..7e297d5b72 100644 --- a/src/dawn/tests/unittests/validation/MultipleDeviceTests.cpp +++ b/src/dawn/tests/unittests/validation/MultipleDeviceTests.cpp @@ -37,7 +37,7 @@ TEST_F(MultipleDeviceTest, ValidatesSameDevice) { // objects from a different device. TEST_F(MultipleDeviceTest, ValidatesSameDeviceCreatePipelineAsync) { wgpu::ShaderModuleWGSLDescriptor wgslDesc = {}; - wgslDesc.source = R"( + wgslDesc.code = R"( @compute @workgroup_size(1, 1, 1) fn main() { } )"; diff --git a/src/dawn/tests/unittests/validation/ShaderModuleValidationTests.cpp b/src/dawn/tests/unittests/validation/ShaderModuleValidationTests.cpp index 05c04a2823..cf7a896b20 100644 --- a/src/dawn/tests/unittests/validation/ShaderModuleValidationTests.cpp +++ b/src/dawn/tests/unittests/validation/ShaderModuleValidationTests.cpp @@ -205,7 +205,7 @@ TEST_F(ShaderModuleValidationTest, MultipleChainedDescriptor_WgslAndSpirv) { spirv_desc.code = &code; spirv_desc.codeSize = 1; wgpu::ShaderModuleWGSLDescriptor wgsl_desc = {}; - wgsl_desc.source = ""; + wgsl_desc.code = ""; wgsl_desc.nextInChain = &spirv_desc; desc.nextInChain = &wgsl_desc; ASSERT_DEVICE_ERROR(device.CreateShaderModule(&desc), @@ -219,7 +219,7 @@ TEST_F(ShaderModuleValidationTest, MultipleChainedDescriptor_WgslAndDawnSpirvOpt wgpu::DawnShaderModuleSPIRVOptionsDescriptor spirv_options_desc = {}; wgpu::ShaderModuleWGSLDescriptor wgsl_desc = {}; wgsl_desc.nextInChain = &spirv_options_desc; - wgsl_desc.source = ""; + wgsl_desc.code = ""; desc.nextInChain = &wgsl_desc; ASSERT_DEVICE_ERROR( device.CreateShaderModule(&desc), @@ -692,3 +692,35 @@ enable f16; @compute @workgroup_size(1) fn main() {})")); } + +// Test that passing a WGSL extension without setting the shader string should fail. +TEST_F(ShaderModuleValidationTest, WgslNullptrShader) { + wgpu::ShaderModuleWGSLDescriptor wgslDesc = {}; + wgpu::ShaderModuleDescriptor descriptor = {}; + descriptor.nextInChain = &wgslDesc; + ASSERT_DEVICE_ERROR(device.CreateShaderModule(&descriptor)); +} + +// Tests that WGSL extension with deprecated source member still works but emits warning. +TEST_F(ShaderModuleValidationTest, SourceToCodeMemberDeprecation) { + // This test works assuming ShaderModule is backed by a dawn::native::ShaderModuleBase, which + // is not the case on the wire. + DAWN_SKIP_TEST_IF(UsesWire()); + + wgpu::ShaderModuleWGSLDescriptor wgslDesc = {}; + wgpu::ShaderModuleDescriptor descriptor = {}; + descriptor.nextInChain = &wgslDesc; + + wgpu::ShaderModule sourceShader; + wgslDesc.source = "@compute @workgroup_size(1) fn main() {}"; + // Note that there are actually 2 warnings emitted because 1 is for the blueprint and one is for + // the actual shader. + EXPECT_DEPRECATION_WARNINGS(sourceShader = device.CreateShaderModule(&descriptor), 2); + + wgslDesc.source = nullptr; + wgslDesc.code = "@compute @workgroup_size(1) fn main() {}"; + wgpu::ShaderModule codeShader = device.CreateShaderModule(&descriptor); + + EXPECT_TRUE(dawn::native::ShaderModuleBase::EqualityFunc()( + dawn::native::FromAPI(sourceShader.Get()), dawn::native::FromAPI(codeShader.Get()))); +} diff --git a/src/dawn/tests/unittests/wire/WireDeviceLifetimeTests.cpp b/src/dawn/tests/unittests/wire/WireDeviceLifetimeTests.cpp index 1b67872864..333a63705f 100644 --- a/src/dawn/tests/unittests/wire/WireDeviceLifetimeTests.cpp +++ b/src/dawn/tests/unittests/wire/WireDeviceLifetimeTests.cpp @@ -144,7 +144,7 @@ TEST_F(WireDeviceLifetimeTests, DeviceDroppedFromWireThenLoggingCallback) { wgpu::ShaderModuleDescriptor shaderModuleDesc = {}; wgpu::ShaderModuleWGSLDescriptor wgslDesc = {}; shaderModuleDesc.nextInChain = &wgslDesc; - wgslDesc.source = "@compute @workgroup_size(64) fn main() {}"; + wgslDesc.code = "@compute @workgroup_size(64) fn main() {}"; // Create a shader module so the transformed shaders are dumped. device.CreateShaderModule(&shaderModuleDesc); diff --git a/src/dawn/utils/WGPUHelpers.cpp b/src/dawn/utils/WGPUHelpers.cpp index e517945fc2..ee9c14bccc 100644 --- a/src/dawn/utils/WGPUHelpers.cpp +++ b/src/dawn/utils/WGPUHelpers.cpp @@ -84,7 +84,7 @@ wgpu::ShaderModule CreateShaderModuleFromASM( wgpu::ShaderModule CreateShaderModule(const wgpu::Device& device, const char* source) { wgpu::ShaderModuleWGSLDescriptor wgslDesc; - wgslDesc.source = source; + wgslDesc.code = source; wgpu::ShaderModuleDescriptor descriptor; descriptor.nextInChain = &wgslDesc; return device.CreateShaderModule(&descriptor);