D3D12: Always skip Robustness transform on non-DBO storage buffers
This patch adds all the storage buffers and read-only storage
buffers without Dynamic Buffer Offset to the 'ignore-list' of Tint
Robustness transform on D3D12 backend because currently in Dawn they
will always be bound to descriptor tables and translated into
ByteAddressBuffers, where D3D12 runtime can guarantee that
out-of-bounds-reading will always return 0 and out-of-bound-writing
will always take no action.
Note that currently we cannot skip robustness transform on uniform
buffers because in some situations FXC will report compilation error
when there is OOB access to the sized array declared in cBuffer.
This CL can improve about 11% of the performance of
dawn_perf_test.ShaderRobustnessPerf.
Bug: tint:1890
Change-Id: I34623fe1fced18208f77983d3eaf89d63afb8aed
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/136340
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Jiawei Shao <jiawei.shao@intel.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/dawn/native/d3d/D3DCompilationRequest.h b/src/dawn/native/d3d/D3DCompilationRequest.h
index 9adebeb..dc12a32 100644
--- a/src/dawn/native/d3d/D3DCompilationRequest.h
+++ b/src/dawn/native/d3d/D3DCompilationRequest.h
@@ -18,6 +18,7 @@
#include <d3dcompiler.h>
#include <string>
+#include <vector>
#include "dawn/native/CacheRequest.h"
#include "dawn/native/Serializable.h"
@@ -70,7 +71,8 @@
X(bool, isRobustnessEnabled) \
X(bool, disableWorkgroupInit) \
X(bool, polyfillReflectVec2F32) \
- X(bool, dumpShaders)
+ X(bool, dumpShaders) \
+ X(std::vector<tint::writer::BindingPoint>, bindingPointsIgnoredInRobustnessTransform)
#define D3D_BYTECODE_COMPILATION_REQUEST_MEMBERS(X) \
X(bool, hasShaderF16Feature) \
diff --git a/src/dawn/native/d3d/ShaderUtils.cpp b/src/dawn/native/d3d/ShaderUtils.cpp
index ddb38b7..37bac70 100644
--- a/src/dawn/native/d3d/ShaderUtils.cpp
+++ b/src/dawn/native/d3d/ShaderUtils.cpp
@@ -239,6 +239,9 @@
options.polyfill_reflect_vec2_f32 = r.polyfillReflectVec2F32;
+ options.binding_points_ignored_in_robustness_transform =
+ std::move(r.bindingPointsIgnoredInRobustnessTransform);
+
TRACE_EVENT0(tracePlatform.UnsafeGetValue(), General, "tint::writer::hlsl::Generate");
auto result = tint::writer::hlsl::Generate(&transformedProgram, options);
DAWN_INVALID_IF(!result.success, "An error occured while generating HLSL: %s", result.error);
diff --git a/src/dawn/native/d3d12/ShaderModuleD3D12.cpp b/src/dawn/native/d3d12/ShaderModuleD3D12.cpp
index 78d9b70..83dced9 100644
--- a/src/dawn/native/d3d12/ShaderModuleD3D12.cpp
+++ b/src/dawn/native/d3d12/ShaderModuleD3D12.cpp
@@ -131,13 +131,13 @@
const BindingInfoArray& moduleBindingInfo = entryPoint.bindings;
for (BindGroupIndex group : IterateBitSet(layout->GetBindGroupLayoutsMask())) {
const BindGroupLayout* bgl = ToBackend(layout->GetBindGroupLayout(group));
- const auto& groupBindingInfo = moduleBindingInfo[group];
+ const auto& moduleGroupBindingInfo = moduleBindingInfo[group];
// d3d12::BindGroupLayout packs the bindings per HLSL register-space. We modify
// the Tint AST to make the "bindings" decoration match the offset chosen by
// d3d12::BindGroupLayout so that Tint produces HLSL with the correct registers
// assigned to each interface variable.
- for (const auto& [binding, bindingInfo] : groupBindingInfo) {
+ for (const auto& [binding, bindingInfo] : moduleGroupBindingInfo) {
BindingIndex bindingIndex = bgl->GetBindingIndex(binding);
BindingPoint srcBindingPoint{static_cast<uint32_t>(group),
static_cast<uint32_t>(binding)};
@@ -160,6 +160,36 @@
bindingRemapper.access_controls.emplace(srcBindingPoint,
tint::builtin::Access::kReadWrite);
}
+
+ // On D3D12 backend all storage buffers without Dynamic Buffer Offset will always be
+ // bound to root descriptor tables, where D3D12 runtime can guarantee that OOB-read will
+ // always return 0 and OOB-write will always take no action, so we don't need to do
+ // robustness transform on them. Note that we still need to do robustness transform on
+ // uniform buffers because only sized array is allowed in uniform buffers, so FXC will
+ // report compilation error when the indexing to the array in a cBuffer is out of bound
+ // and can be checked at compilation time. Storage buffers are OK because they are
+ // always translated with RWByteAddressBuffers, which has no such sized arrays.
+ //
+ // For example below WGSL shader will cause compilation error when we skip robustness
+ // transform on uniform buffers:
+ //
+ // struct TestData {
+ // data: array<vec4<u32>, 3>,
+ // };
+ // @group(0) @binding(0) var<uniform> s: TestData;
+ //
+ // fn test() -> u32 {
+ // let index = 1000000u;
+ // if (s.data[index][0] != 0u) { // error X3504: array index out of bounds
+ // return 0x1004u;
+ // }
+ // return 0u;
+ // }
+ if ((bindingInfo.buffer.type == wgpu::BufferBindingType::Storage ||
+ bindingInfo.buffer.type == wgpu::BufferBindingType::ReadOnlyStorage) &&
+ !bgl->GetBindingInfo(bindingIndex).buffer.hasDynamicOffset) {
+ req.hlsl.bindingPointsIgnoredInRobustnessTransform.emplace_back(srcBindingPoint);
+ }
}
// Add arrayLengthFromUniform options
diff --git a/src/tint/ast/transform/robustness.cc b/src/tint/ast/transform/robustness.cc
index 120d256..aa94748 100644
--- a/src/tint/ast/transform/robustness.cc
+++ b/src/tint/ast/transform/robustness.cc
@@ -62,6 +62,9 @@
// obj[idx]
// Array, matrix and vector indexing may require robustness transformation.
auto* expr = sem.Get(e)->Unwrap()->As<sem::IndexAccessorExpression>();
+ if (IsIgnoredResourceBinding(expr->Object()->RootIdentifier())) {
+ return;
+ }
switch (ActionFor(expr)) {
case Action::kPredicate:
PredicateIndexAccessor(expr);
@@ -673,6 +676,22 @@
const CallExpression* CastToUnsigned(const Expression* val, uint32_t width) {
return b.Call(ScalarOrVecTy(b.ty.u32(), width), val);
}
+
+ /// @returns true if the variable represents a resource binding that should be ignored in the
+ /// robustness check.
+ /// TODO(tint:1890): make this function work with unrestricted pointer paramters. Note that this
+ /// depends on transform::DirectVariableAccess to have been run first.
+ bool IsIgnoredResourceBinding(const sem::Variable* variable) const {
+ auto* globalVariable = utils::As<sem::GlobalVariable>(variable);
+ if (globalVariable == nullptr) {
+ return false;
+ }
+ if (!globalVariable->BindingPoint().has_value()) {
+ return false;
+ }
+ sem::BindingPoint bindingPoint = *globalVariable->BindingPoint();
+ return cfg.bindings_ignored.find(bindingPoint) != cfg.bindings_ignored.cend();
+ }
};
Robustness::Config::Config() = default;
diff --git a/src/tint/ast/transform/robustness.h b/src/tint/ast/transform/robustness.h
index 35804b4..7278a41 100644
--- a/src/tint/ast/transform/robustness.h
+++ b/src/tint/ast/transform/robustness.h
@@ -15,7 +15,10 @@
#ifndef SRC_TINT_AST_TRANSFORM_ROBUSTNESS_H_
#define SRC_TINT_AST_TRANSFORM_ROBUSTNESS_H_
+#include <unordered_set>
+
#include "src/tint/ast/transform/transform.h"
+#include "src/tint/sem/binding_point.h"
namespace tint::ast::transform {
@@ -77,6 +80,9 @@
Action uniform_action = Action::kDefault;
/// Robustness action for variables in the 'workgroup' address space
Action workgroup_action = Action::kDefault;
+
+ /// Bindings that should always be applied Actions::kIgnore on.
+ std::unordered_set<tint::sem::BindingPoint> bindings_ignored;
};
/// Constructor
diff --git a/src/tint/writer/hlsl/generator.h b/src/tint/writer/hlsl/generator.h
index 804b449..9fbc01c 100644
--- a/src/tint/writer/hlsl/generator.h
+++ b/src/tint/writer/hlsl/generator.h
@@ -80,6 +80,9 @@
/// Set to `true` to generate polyfill for `reflect` builtin for vec2<f32>
bool polyfill_reflect_vec2_f32 = false;
+ /// The binding points that will be ignored in the rebustness transform.
+ std::vector<sem::BindingPoint> binding_points_ignored_in_robustness_transform;
+
/// Reflect the fields of this class so that it can be used by tint::ForeachField()
TINT_REFLECT(disable_robustness,
root_constant_binding_point,
diff --git a/src/tint/writer/hlsl/generator_impl.cc b/src/tint/writer/hlsl/generator_impl.cc
index 79b091c..1b30d8d 100644
--- a/src/tint/writer/hlsl/generator_impl.cc
+++ b/src/tint/writer/hlsl/generator_impl.cc
@@ -192,6 +192,12 @@
// Robustness must come after PromoteSideEffectsToDecl
// Robustness must come before BuiltinPolyfill and CanonicalizeEntryPointIO
manager.Add<ast::transform::Robustness>();
+
+ ast::transform::Robustness::Config config = {};
+ config.bindings_ignored = std::unordered_set<sem::BindingPoint>(
+ options.binding_points_ignored_in_robustness_transform.cbegin(),
+ options.binding_points_ignored_in_robustness_transform.cend());
+ data.Add<ast::transform::Robustness::Config>(config);
}
// Note: it is more efficient for MultiplanarExternalTexture to come after Robustness
