Vulkan: Skip index clamping on runtime-sized arrays in robustness transform
This patch disables index clamping on all runtime-sized arrays in robustness
transform when VK_EXT_robustness2 is supported and robustBufferAccess2 is
VK_TRUE.
This patch can improve about 11% of the performance of
dawn_perf_tests.ShaderRobustnessPerf.
Fixed: dawn:1882
Bug: tint:1890
Change-Id: Ib7113c91787a51c34fdc3496f3d700cb1d74e709
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/139760
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Jiawei Shao <jiawei.shao@intel.com>
diff --git a/src/dawn/native/Toggles.cpp b/src/dawn/native/Toggles.cpp
index e5ceb56..b551e51 100644
--- a/src/dawn/native/Toggles.cpp
+++ b/src/dawn/native/Toggles.cpp
@@ -430,6 +430,11 @@
"Disable Tint robustness transform on textures when VK_EXT_robustness2 is supported and "
"robustImageAccess2 == VK_TRUE.",
"https://crbug.com/tint/1890", ToggleStage::Device}},
+ {Toggle::VulkanUseBufferRobustAccess2,
+ {"vulkan_use_buffer_robust_access_2",
+ "Disable index clamping on the runtime-sized arrays on buffers in Tint robustness transform "
+ "when VK_EXT_robustness2 is supported and robustBufferAccess2 == VK_TRUE.",
+ "https://crbug.com/tint/1890", ToggleStage::Device}},
{Toggle::NoWorkaroundSampleMaskBecomesZeroForAllButLastColorTarget,
{"no_workaround_sample_mask_becomes_zero_for_all_but_last_color_target",
"MacOS 12.0+ Intel has a bug where the sample mask is only applied for the last color "
diff --git a/src/dawn/native/Toggles.h b/src/dawn/native/Toggles.h
index 271b5c9..c7061a7 100644
--- a/src/dawn/native/Toggles.h
+++ b/src/dawn/native/Toggles.h
@@ -100,6 +100,7 @@
VulkanClearGen12TextureWithCCSAmbiguateOnCreation,
D3D12UseRootSignatureVersion1_1,
VulkanUseImageRobustAccess2,
+ VulkanUseBufferRobustAccess2,
// Unresolved issues.
NoWorkaroundSampleMaskBecomesZeroForAllButLastColorTarget,
diff --git a/src/dawn/native/vulkan/DeviceVk.cpp b/src/dawn/native/vulkan/DeviceVk.cpp
index 9617771..fbe8172 100644
--- a/src/dawn/native/vulkan/DeviceVk.cpp
+++ b/src/dawn/native/vulkan/DeviceVk.cpp
@@ -524,9 +524,6 @@
ASSERT(usedKnobs.HasExt(DeviceExt::Robustness2));
usedKnobs.robustness2Features = mDeviceInfo.robustness2Features;
- // TODO(tint:1890): investigate how we can safely disable buffer access in Tint when
- // robustBufferAccess2 == TRUE
- usedKnobs.robustness2Features.robustBufferAccess2 = VK_FALSE;
featuresChain.Add(&usedKnobs.robustness2Features);
}
diff --git a/src/dawn/native/vulkan/PhysicalDeviceVk.cpp b/src/dawn/native/vulkan/PhysicalDeviceVk.cpp
index 3ebb617..b5d3d56 100644
--- a/src/dawn/native/vulkan/PhysicalDeviceVk.cpp
+++ b/src/dawn/native/vulkan/PhysicalDeviceVk.cpp
@@ -565,6 +565,15 @@
// By default try to skip robustness transform on textures according to the Vulkan extension
// VK_EXT_robustness2.
deviceToggles->Default(Toggle::VulkanUseImageRobustAccess2, true);
+ // The environment can only request to use VK_EXT_robustness2 when the extension is available.
+ // Override the decision if it is not applicable or robustBufferAccess2 is false.
+ if (!GetDeviceInfo().HasExt(DeviceExt::Robustness2) ||
+ GetDeviceInfo().robustness2Features.robustBufferAccess2 == VK_FALSE) {
+ deviceToggles->ForceSet(Toggle::VulkanUseBufferRobustAccess2, false);
+ }
+ // By default try to disable index clamping on the runtime-sized arrays on storage buffers in
+ // Tint robustness transform according to the Vulkan extension VK_EXT_robustness2.
+ deviceToggles->Default(Toggle::VulkanUseBufferRobustAccess2, true);
}
ResultOrError<Ref<DeviceBase>> PhysicalDevice::CreateDeviceImpl(AdapterBase* adapter,
diff --git a/src/dawn/native/vulkan/ShaderModuleVk.cpp b/src/dawn/native/vulkan/ShaderModuleVk.cpp
index 55f5911..4074ba5 100644
--- a/src/dawn/native/vulkan/ShaderModuleVk.cpp
+++ b/src/dawn/native/vulkan/ShaderModuleVk.cpp
@@ -177,6 +177,7 @@
X(bool, useZeroInitializeWorkgroupMemoryExtension) \
X(bool, clampFragDepth) \
X(bool, disableImageRobustness) \
+ X(bool, disableRuntimeSizedArrayIndexClamping) \
X(CacheKey::UnsafeUnkeyedValue<dawn::platform::Platform*>, tracePlatform)
DAWN_MAKE_CACHE_REQUEST(SpirvCompilationRequest, SPIRV_COMPILATION_REQUEST_MEMBERS);
@@ -264,6 +265,10 @@
GetDevice()->IsToggleEnabled(Toggle::VulkanUseZeroInitializeWorkgroupMemoryExtension);
req.clampFragDepth = clampFragDepth;
req.disableImageRobustness = GetDevice()->IsToggleEnabled(Toggle::VulkanUseImageRobustAccess2);
+ // Currently we can disable index clamping on all runtime-sized arrays in Tint robustness
+ // transform as unsized arrays can only be declared on storage address space.
+ req.disableRuntimeSizedArrayIndexClamping =
+ GetDevice()->IsToggleEnabled(Toggle::VulkanUseBufferRobustAccess2);
req.tracePlatform = UnsafeUnkeyedValue(GetDevice()->GetPlatform());
req.substituteOverrideConfig = std::move(substituteOverrideConfig);
@@ -336,6 +341,8 @@
options.binding_remapper_options = r.bindingRemapper;
options.external_texture_options = r.externalTextureOptions;
options.disable_image_robustness = r.disableImageRobustness;
+ options.disable_runtime_sized_array_index_clamping =
+ r.disableRuntimeSizedArrayIndexClamping;
TRACE_EVENT0(r.tracePlatform.UnsafeGetValue(), General,
"tint::writer::spirv::Generate()");
diff --git a/src/tint/ast/transform/robustness.cc b/src/tint/ast/transform/robustness.cc
index e0cb752..1e9a4a0 100644
--- a/src/tint/ast/transform/robustness.cc
+++ b/src/tint/ast/transform/robustness.cc
@@ -66,6 +66,14 @@
if (IsIgnoredResourceBinding(expr->Object()->RootIdentifier())) {
return;
}
+ if (cfg.disable_runtime_sized_array_index_clamping &&
+ IsIndexAccessingRuntimeSizedArray(expr)) {
+ // Ensure the index is always u32 as using a negative index is an undefined
+ // behavior in SPIRV.
+ auto* idx = CastToU32(expr->Index());
+ ctx.Replace(expr->Declaration()->index, idx);
+ return;
+ }
switch (ActionFor(expr)) {
case Action::kPredicate:
PredicateIndexAccessor(expr);
@@ -342,11 +350,7 @@
}
auto* expr_sem = expr->Unwrap()->As<sem::IndexAccessorExpression>();
-
- auto idx = ctx.Clone(expr->Declaration()->index);
- if (expr_sem->Index()->Type()->is_signed_integer_scalar()) {
- idx = b.Call<u32>(idx); // u32(idx)
- }
+ auto idx = CastToU32(expr_sem->Index());
auto* clamped_idx = b.Call(builtin::Function::kMin, idx, max);
ctx.Replace(expr->Declaration()->index, clamped_idx);
}
@@ -693,6 +697,22 @@
sem::BindingPoint bindingPoint = *globalVariable->BindingPoint();
return cfg.bindings_ignored.find(bindingPoint) != cfg.bindings_ignored.cend();
}
+
+ /// @returns true if expr is an IndexAccessorExpression whose object is a runtime-sized array.
+ bool IsIndexAccessingRuntimeSizedArray(const sem::IndexAccessorExpression* expr) {
+ auto* array_type = expr->Object()->Type()->UnwrapRef()->As<type::Array>();
+ return array_type != nullptr && array_type->Count()->Is<type::RuntimeArrayCount>();
+ }
+
+ /// @returns a clone of expr->Declaration() if it is an unsigned integer scalar, or
+ /// expr->Declaration() cast to u32.
+ const ast::Expression* CastToU32(const sem::ValueExpression* expr) {
+ auto* idx = ctx.Clone(expr->Declaration());
+ if (expr->Type()->is_unsigned_integer_scalar()) {
+ return idx;
+ }
+ return b.Call<u32>(idx); // u32(idx)
+ }
};
Robustness::Config::Config() = default;
diff --git a/src/tint/ast/transform/robustness.h b/src/tint/ast/transform/robustness.h
index 7278a41..be01bb5 100644
--- a/src/tint/ast/transform/robustness.h
+++ b/src/tint/ast/transform/robustness.h
@@ -81,8 +81,11 @@
/// Robustness action for variables in the 'workgroup' address space
Action workgroup_action = Action::kDefault;
- /// Bindings that should always be applied Actions::kIgnore on.
+ /// Bindings that should always be applied Actions::kIgnore on
std::unordered_set<tint::sem::BindingPoint> bindings_ignored;
+
+ /// If we should disable index clamping on runtime-sized arrays in robustness transform
+ bool disable_runtime_sized_array_index_clamping = false;
};
/// Constructor
diff --git a/src/tint/ast/transform/robustness_test.cc b/src/tint/ast/transform/robustness_test.cc
index ebe7dc2..0b7e15c 100644
--- a/src/tint/ast/transform/robustness_test.cc
+++ b/src/tint/ast/transform/robustness_test.cc
@@ -32,7 +32,8 @@
namespace {
-Transform::DataMap Config(Robustness::Action action) {
+Transform::DataMap Config(Robustness::Action action,
+ bool disable_runtime_sized_array_index_clamping = false) {
Robustness::Config cfg;
cfg.value_action = action;
cfg.texture_action = action;
@@ -42,6 +43,7 @@
cfg.storage_action = action;
cfg.uniform_action = action;
cfg.workgroup_action = action;
+ cfg.disable_runtime_sized_array_index_clamping = disable_runtime_sized_array_index_clamping;
Transform::DataMap data;
data.Add<Robustness::Config>(cfg);
return data;
@@ -5490,6 +5492,136 @@
EXPECT_EQ(expect, str(got));
}
+////////////////////////////////////////////////////////////////////////////////
+// disable_runtime_sized_array_index_clamping == true
+////////////////////////////////////////////////////////////////////////////////
+
+TEST_P(RobustnessTest, Read_disable_unsized_array_index_clamping_i32) {
+ auto* src = R"(
+@group(0) @binding(0) var<storage, read> s : array<f32>;
+
+fn f() {
+ let i = 25i;
+ var d : f32 = s[i];
+}
+)";
+
+ auto* expect = R"(
+@group(0) @binding(0) var<storage, read> s : array<f32>;
+
+fn f() {
+ let i = 25i;
+ var d : f32 = s[u32(i)];
+}
+)";
+
+ auto got = Run<Robustness>(src, Config(GetParam(), true));
+
+ EXPECT_EQ(expect, str(got));
+}
+
+TEST_P(RobustnessTest, Read_disable_unsized_array_index_clamping_u32) {
+ auto* src = R"(
+@group(0) @binding(0) var<storage, read> s : array<f32>;
+
+fn f() {
+ let i = 25u;
+ var d : f32 = s[i];
+}
+)";
+
+ auto* expect = src;
+
+ auto got = Run<Robustness>(src, Config(GetParam(), true));
+
+ EXPECT_EQ(expect, str(got));
+}
+
+TEST_P(RobustnessTest, Read_disable_unsized_array_index_clamping_abstract_int) {
+ auto* src = R"(
+@group(0) @binding(0) var<storage, read> s : array<f32>;
+
+fn f() {
+ var d : f32 = s[25];
+}
+)";
+
+ auto* expect = R"(
+@group(0) @binding(0) var<storage, read> s : array<f32>;
+
+fn f() {
+ var d : f32 = s[u32(25)];
+}
+)";
+
+ auto got = Run<Robustness>(src, Config(GetParam(), true));
+
+ EXPECT_EQ(expect, str(got));
+}
+
+TEST_P(RobustnessTest, Assign_disable_unsized_array_index_clamping_i32) {
+ auto* src = R"(
+@group(0) @binding(0) var<storage, read_write> s : array<f32>;
+
+fn f() {
+ let i = 25i;
+ s[i] = 0.5f;
+}
+)";
+
+ auto* expect = R"(
+@group(0) @binding(0) var<storage, read_write> s : array<f32>;
+
+fn f() {
+ let i = 25i;
+ s[u32(i)] = 0.5f;
+}
+)";
+
+ auto got = Run<Robustness>(src, Config(GetParam(), true));
+
+ EXPECT_EQ(expect, str(got));
+}
+
+TEST_P(RobustnessTest, Assign_disable_unsized_array_index_clamping_u32) {
+ auto* src = R"(
+@group(0) @binding(0) var<storage, read_write> s : array<f32>;
+
+fn f() {
+ let i = 25u;
+ s[i] = 0.5f;
+}
+)";
+
+ auto* expect = src;
+
+ auto got = Run<Robustness>(src, Config(GetParam(), true));
+
+ EXPECT_EQ(expect, str(got));
+}
+
+TEST_P(RobustnessTest, Assign_disable_unsized_array_index_clamping_abstract_int) {
+ auto* src = R"(
+@group(0) @binding(0) var<storage, read_write> s : array<f32>;
+
+fn f() {
+ s[25] = 0.5f;
+}
+)";
+
+ auto* expect = R"(
+@group(0) @binding(0) var<storage, read_write> s : array<f32>;
+
+fn f() {
+ s[u32(25)] = 0.5f;
+}
+)";
+
+ auto got = Run<Robustness>(src, Config(GetParam(), true));
+
+ EXPECT_EQ(expect, str(got));
+}
+
INSTANTIATE_TEST_SUITE_P(,
RobustnessTest,
testing::Values(Robustness::Action::kIgnore,
diff --git a/src/tint/writer/spirv/generator.h b/src/tint/writer/spirv/generator.h
index 3319c05..f9bc2c2 100644
--- a/src/tint/writer/spirv/generator.h
+++ b/src/tint/writer/spirv/generator.h
@@ -60,10 +60,12 @@
/// VK_KHR_zero_initialize_workgroup_memory is enabled.
bool use_zero_initialize_workgroup_memory_extension = false;
- /// Set to `true` to skip robustness transform on textures when VK_EXT_robustness is enabled and
- /// robustImageAccess == VK_TRUE.
+ /// Set to `true` to skip robustness transform on textures.
bool disable_image_robustness = false;
+ /// Set to `true` to disable index clamping on the runtime-sized arrays in robustness transform.
+ bool disable_runtime_sized_array_index_clamping = false;
+
#if TINT_BUILD_IR
/// Set to `true` to generate SPIR-V via the Tint IR instead of from the AST.
bool use_tint_ir = false;
@@ -75,7 +77,8 @@
disable_workgroup_init,
external_texture_options,
use_zero_initialize_workgroup_memory_extension,
- disable_image_robustness);
+ disable_image_robustness,
+ disable_runtime_sized_array_index_clamping);
};
/// The result produced when generating SPIR-V.
diff --git a/src/tint/writer/spirv/generator_impl.cc b/src/tint/writer/spirv/generator_impl.cc
index b5e5a5c..edab51c 100644
--- a/src/tint/writer/spirv/generator_impl.cc
+++ b/src/tint/writer/spirv/generator_impl.cc
@@ -87,6 +87,8 @@
if (options.disable_image_robustness) {
config.texture_action = ast::transform::Robustness::Action::kIgnore;
}
+ config.disable_runtime_sized_array_index_clamping =
+ options.disable_runtime_sized_array_index_clamping;
data.Add<ast::transform::Robustness::Config>(config);
}
