D3D12 fix for register binding offsets. When both the vert and frag shaders have a UBO binding, the D3D12 backend was using register offset 0 for both, causing a collision, and the wrong constant value used in one of the shaders. The fix is to use the binding offsets computed by the BindGroupLayout, since they know about all of the bindings, not just the ones computed for each shader. This made it necessary to defer shader compilation until pipeline layout creation time (as is done in the Metal backend for similar reasons). Finally, those bindings offsets computed by the BGL include an offset for the CBV, UAV and SRV subgroups, so we must add the same register offset when assigning the BaseShaderRegister to the descriptor ranges in the PipelineLayout constructor so that they match. Bug: dawn:20 Change-Id: I18287bf1c06f06dd61288e12da64752f54634466 Reviewed-on: https://dawn-review.googlesource.com/c/1960 Reviewed-by: Stephen White <senorblanco@chromium.org> Reviewed-by: Corentin Wallez <cwallez@chromium.org> Commit-Queue: Stephen White <senorblanco@chromium.org>
diff --git a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp index 91cd51f..7477e3f 100644 --- a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp +++ b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp
@@ -40,7 +40,7 @@ } } - auto SetDescriptorRange = [&](uint32_t index, uint32_t count, + auto SetDescriptorRange = [&](uint32_t index, uint32_t count, uint32_t* baseRegister, D3D12_DESCRIPTOR_RANGE_TYPE type) -> bool { if (count == 0) { return false; @@ -51,36 +51,35 @@ range.NumDescriptors = count; range.RegisterSpace = 0; range.OffsetInDescriptorsFromTableStart = D3D12_DESCRIPTOR_RANGE_OFFSET_APPEND; + range.BaseShaderRegister = *baseRegister; + *baseRegister += count; // These ranges will be copied and range.BaseShaderRegister will be set in // d3d12::PipelineLayout to account for bind group register offsets return true; }; uint32_t rangeIndex = 0; + uint32_t baseRegister = 0; + std::array<uint32_t, DescriptorType::Count> descriptorOffsets; // Ranges 0-2 contain the CBV, UAV, and SRV ranges, if they exist, tightly packed // Range 3 contains the Sampler range, if there is one - if (SetDescriptorRange(rangeIndex, mDescriptorCounts[CBV], + if (SetDescriptorRange(rangeIndex, mDescriptorCounts[CBV], &baseRegister, D3D12_DESCRIPTOR_RANGE_TYPE_CBV)) { - rangeIndex++; + descriptorOffsets[CBV] = mRanges[rangeIndex++].BaseShaderRegister; } - if (SetDescriptorRange(rangeIndex, mDescriptorCounts[UAV], + if (SetDescriptorRange(rangeIndex, mDescriptorCounts[UAV], &baseRegister, D3D12_DESCRIPTOR_RANGE_TYPE_UAV)) { - rangeIndex++; + descriptorOffsets[UAV] = mRanges[rangeIndex++].BaseShaderRegister; } - if (SetDescriptorRange(rangeIndex, mDescriptorCounts[SRV], + if (SetDescriptorRange(rangeIndex, mDescriptorCounts[SRV], &baseRegister, D3D12_DESCRIPTOR_RANGE_TYPE_SRV)) { - rangeIndex++; + descriptorOffsets[SRV] = mRanges[rangeIndex++].BaseShaderRegister; } - SetDescriptorRange(Sampler, mDescriptorCounts[Sampler], + uint32_t zero = 0; + SetDescriptorRange(Sampler, mDescriptorCounts[Sampler], &zero, D3D12_DESCRIPTOR_RANGE_TYPE_SAMPLER); - - // descriptors ranges are offset by the offset + size of the previous range - std::array<uint32_t, DescriptorType::Count> descriptorOffsets; - descriptorOffsets[CBV] = 0; - descriptorOffsets[UAV] = descriptorOffsets[CBV] + mDescriptorCounts[CBV]; - descriptorOffsets[SRV] = descriptorOffsets[UAV] + mDescriptorCounts[UAV]; - descriptorOffsets[Sampler] = 0; // samplers are in a different heap + descriptorOffsets[Sampler] = 0; for (uint32_t binding : IterateBitSet(groupInfo.mask)) { switch (groupInfo.types[binding]) {
diff --git a/src/dawn_native/d3d12/ComputePipelineD3D12.cpp b/src/dawn_native/d3d12/ComputePipelineD3D12.cpp index edcbd93..67f4cbc 100644 --- a/src/dawn_native/d3d12/ComputePipelineD3D12.cpp +++ b/src/dawn_native/d3d12/ComputePipelineD3D12.cpp
@@ -33,7 +33,7 @@ compileFlags |= D3DCOMPILE_PACK_MATRIX_ROW_MAJOR; const ShaderModule* module = ToBackend(descriptor->module); - const std::string& hlslSource = module->GetHLSLSource(); + const std::string& hlslSource = module->GetHLSLSource(ToBackend(GetLayout())); ComPtr<ID3DBlob> compiledShader; ComPtr<ID3DBlob> errors;
diff --git a/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp b/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp index ee6e992..1ee21e1 100644 --- a/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp +++ b/src/dawn_native/d3d12/PipelineLayoutD3D12.cpp
@@ -65,7 +65,7 @@ for (uint32_t i = 0; i < rangeCount; ++i) { ranges[rangeIndex] = descriptorRanges[i]; - ranges[rangeIndex].BaseShaderRegister = group * kMaxBindingsPerGroup; + ranges[rangeIndex].BaseShaderRegister += group * kMaxBindingsPerGroup; rangeIndex++; }
diff --git a/src/dawn_native/d3d12/RenderPipelineD3D12.cpp b/src/dawn_native/d3d12/RenderPipelineD3D12.cpp index b375a30..63385db 100644 --- a/src/dawn_native/d3d12/RenderPipelineD3D12.cpp +++ b/src/dawn_native/d3d12/RenderPipelineD3D12.cpp
@@ -83,7 +83,7 @@ for (auto stage : IterateStages(GetStageMask())) { const auto& module = ToBackend(builder->GetStageInfo(stage).module); const auto& entryPoint = builder->GetStageInfo(stage).entryPoint; - const auto& hlslSource = module->GetHLSLSource(); + const auto& hlslSource = module->GetHLSLSource(ToBackend(GetLayout())); const char* compileTarget = nullptr;
diff --git a/src/dawn_native/d3d12/ShaderModuleD3D12.cpp b/src/dawn_native/d3d12/ShaderModuleD3D12.cpp index f9c971e..c4bd12e 100644 --- a/src/dawn_native/d3d12/ShaderModuleD3D12.cpp +++ b/src/dawn_native/d3d12/ShaderModuleD3D12.cpp
@@ -15,39 +15,23 @@ #include "dawn_native/d3d12/ShaderModuleD3D12.h" #include "common/Assert.h" +#include "dawn_native/d3d12/BindGroupLayoutD3D12.h" #include "dawn_native/d3d12/DeviceD3D12.h" +#include "dawn_native/d3d12/PipelineLayoutD3D12.h" #include <spirv-cross/spirv_hlsl.hpp> namespace dawn_native { namespace d3d12 { - // TODO(kainino@chromium.org): Consider replacing this with a generic enum_map. - template <typename T> - class BindingTypeMap { - public: - T& operator[](dawn::BindingType type) { - switch (type) { - case dawn::BindingType::UniformBuffer: - return mMap[0]; - case dawn::BindingType::Sampler: - return mMap[1]; - case dawn::BindingType::SampledTexture: - return mMap[2]; - case dawn::BindingType::StorageBuffer: - return mMap[3]; - default: - DAWN_UNREACHABLE(); - } - } - - private: - static constexpr int kNumBindingTypes = 4; - std::array<T, kNumBindingTypes> mMap{}; - }; - ShaderModule::ShaderModule(Device* device, const ShaderModuleDescriptor* descriptor) : ShaderModuleBase(device, descriptor) { - spirv_cross::CompilerHLSL compiler(descriptor->code, descriptor->codeSize); + mSpirv.assign(descriptor->code, descriptor->code + descriptor->codeSize); + spirv_cross::CompilerHLSL compiler(mSpirv); + ExtractSpirvInfo(compiler); + } + + const std::string ShaderModule::GetHLSLSource(PipelineLayout* layout) const { + spirv_cross::CompilerHLSL compiler(mSpirv); spirv_cross::CompilerGLSL::Options options_glsl; options_glsl.vertex.fixup_clipspace = true; @@ -58,30 +42,22 @@ options_hlsl.shader_model = 51; compiler.set_hlsl_options(options_hlsl); - ExtractSpirvInfo(compiler); - - // rename bindings so that each register type c/u/t/s starts at 0 and then offset by - // kMaxBindingsPerGroup * bindGroupIndex - const auto& moduleBindingInfo = GetBindingInfo(); + const ModuleBindingInfo& moduleBindingInfo = GetBindingInfo(); for (uint32_t group = 0; group < moduleBindingInfo.size(); ++group) { + const auto& bindingOffsets = + ToBackend(layout->GetBindGroupLayout(group))->GetBindingOffsets(); const auto& groupBindingInfo = moduleBindingInfo[group]; - - BindingTypeMap<uint32_t> baseRegisters{}; - for (const auto& bindingInfo : groupBindingInfo) { + for (uint32_t binding = 0; binding < groupBindingInfo.size(); ++binding) { + const BindingInfo& bindingInfo = groupBindingInfo[binding]; if (bindingInfo.used) { - uint32_t& baseRegister = baseRegisters[bindingInfo.type]; uint32_t bindGroupOffset = group * kMaxBindingsPerGroup; + uint32_t bindingOffset = bindingOffsets[binding]; compiler.set_decoration(bindingInfo.id, spv::DecorationBinding, - bindGroupOffset + baseRegister++); + bindGroupOffset + bindingOffset); } } } - - mHlslSource = compiler.compile(); - } - - const std::string& ShaderModule::GetHLSLSource() const { - return mHlslSource; + return compiler.compile(); } }} // namespace dawn_native::d3d12
diff --git a/src/dawn_native/d3d12/ShaderModuleD3D12.h b/src/dawn_native/d3d12/ShaderModuleD3D12.h index 11065c1..7cafd1c 100644 --- a/src/dawn_native/d3d12/ShaderModuleD3D12.h +++ b/src/dawn_native/d3d12/ShaderModuleD3D12.h
@@ -20,15 +20,16 @@ namespace dawn_native { namespace d3d12 { class Device; + class PipelineLayout; class ShaderModule : public ShaderModuleBase { public: ShaderModule(Device* device, const ShaderModuleDescriptor* descriptor); - const std::string& GetHLSLSource() const; + const std::string GetHLSLSource(PipelineLayout* layout) const; private: - std::string mHlslSource; + std::vector<uint32_t> mSpirv; }; }} // namespace dawn_native::d3d12