[dawn] Enable module constant hoisting transform for apple silicon
The module constant hosting code works for apple silicon but has
has issues on Intel/AMD for f16(half). So sadly for now the
module scope hosting code must be conditioned on device.
This new end2end test should go green
https://dawn-review.googlesource.com/c/dawn/+/243854?tab=checks
Bug: 419804339
Change-Id: Iccf18363b0bbc74e876b40fa4dc171d7c2889c64
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/244234
Reviewed-by: James Price <jrprice@google.com>
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Commit-Queue: Peter McNeeley <petermcneeley@google.com>
diff --git a/src/dawn/native/Toggles.cpp b/src/dawn/native/Toggles.cpp
index 7c8ce6d..f9382bc 100644
--- a/src/dawn/native/Toggles.cpp
+++ b/src/dawn/native/Toggles.cpp
@@ -570,6 +570,9 @@
{"disable_polyfills_on_integer_div_and_mod",
"Disable the Tint polyfills on integer division and modulo.", "https://crbug.com/tint/2128",
ToggleStage::Device}},
+ {Toggle::MetalEnableModuleConstant,
+ {"metal_enable_module_constant_transform", "Enable the module constant transform.",
+ "https://crbug.com/419804339", ToggleStage::Device}},
{Toggle::EnableImmediateErrorHandling,
{"enable_immediate_error_handling",
"Have the uncaptured error callback invoked immediately when an error occurs, rather than "
diff --git a/src/dawn/native/Toggles.h b/src/dawn/native/Toggles.h
index ee2a54e..e2ea5c6 100644
--- a/src/dawn/native/Toggles.h
+++ b/src/dawn/native/Toggles.h
@@ -138,6 +138,7 @@
ExposeWGSLTestingFeatures,
ExposeWGSLExperimentalFeatures,
DisablePolyfillsOnIntegerDivisonAndModulo,
+ MetalEnableModuleConstant,
EnableImmediateErrorHandling,
VulkanUseStorageInputOutput16,
D3D12DontUseShaderModel66OrHigher,
diff --git a/src/dawn/native/metal/PhysicalDeviceMTL.mm b/src/dawn/native/metal/PhysicalDeviceMTL.mm
index 9e68f10..efdbcb3e 100644
--- a/src/dawn/native/metal/PhysicalDeviceMTL.mm
+++ b/src/dawn/native/metal/PhysicalDeviceMTL.mm
@@ -29,6 +29,7 @@
#include "dawn/common/CoreFoundationRef.h"
#include "dawn/common/GPUInfo.h"
+#include "dawn/common/GPUInfo_autogen.h"
#include "dawn/common/Log.h"
#include "dawn/common/NSRef.h"
#include "dawn/common/Platform.h"
@@ -441,6 +442,13 @@
deviceToggles->Default(Toggle::MetalRenderR8RG8UnormSmallMipToTempTexture, true);
}
+ // chromium:419804339: Module constant hoisting is broadly available as a msl transform but
+ // there are execution correction issues with f16 for non apple silicon (Intel/AMD). Therefore
+ // we only enable for apple silicon for now.
+ if (gpu_info::IsApple(vendorId)) {
+ deviceToggles->Default(Toggle::MetalEnableModuleConstant, true);
+ }
+
// On some Intel GPUs vertex only render pipeline get wrong depth result if no fragment
// shader provided. Create a placeholder fragment shader module to work around this issue.
if (gpu_info::IsIntel(vendorId)) {
diff --git a/src/dawn/native/metal/ShaderModuleMTL.mm b/src/dawn/native/metal/ShaderModuleMTL.mm
index 618677a..4b96fa6 100644
--- a/src/dawn/native/metal/ShaderModuleMTL.mm
+++ b/src/dawn/native/metal/ShaderModuleMTL.mm
@@ -298,6 +298,8 @@
req.tintOptions.bindings = std::move(bindings);
req.tintOptions.disable_polyfill_integer_div_mod =
device->IsToggleEnabled(Toggle::DisablePolyfillsOnIntegerDivisonAndModulo);
+ req.tintOptions.enable_module_constant =
+ device->IsToggleEnabled(Toggle::MetalEnableModuleConstant);
req.tintOptions.vertex_pulling_config = std::move(vertexPullingTransformConfig);
req.tintOptions.enable_integer_range_analysis =
device->IsToggleEnabled(Toggle::EnableIntegerRangeAnalysisInRobustness);
diff --git a/src/tint/lang/msl/writer/common/options.h b/src/tint/lang/msl/writer/common/options.h
index 899e2d2..7dac767 100644
--- a/src/tint/lang/msl/writer/common/options.h
+++ b/src/tint/lang/msl/writer/common/options.h
@@ -163,6 +163,9 @@
/// Set to `true` to disable the polyfills on integer division and modulo.
bool disable_polyfill_integer_div_mod = false;
+ /// Set to `true` to enable the module constant transform
+ bool enable_module_constant = false;
+
/// Emit argument buffers
bool use_argument_buffers = false;
@@ -197,6 +200,7 @@
disable_demote_to_helper,
emit_vertex_point_size,
disable_polyfill_integer_div_mod,
+ enable_module_constant,
use_argument_buffers,
buffer_size_ubo_index,
fixed_sample_mask,
diff --git a/src/tint/lang/msl/writer/printer/printer.cc b/src/tint/lang/msl/writer/printer/printer.cc
index b9df098..84e2c43 100644
--- a/src/tint/lang/msl/writer/printer/printer.cc
+++ b/src/tint/lang/msl/writer/printer/printer.cc
@@ -662,7 +662,9 @@
if (current_function_ == nullptr) {
// program scope let
- out << "constexpr constant ";
+ // TODO(crbug.com/419804339): This should be 'constexpr constant' but the matrix class
+ // (constructor) in metal is not constexpr.
+ out << "const constant ";
}
EmitType(out, l->Result()->Type());
out << " ";
diff --git a/src/tint/lang/msl/writer/raise/raise.cc b/src/tint/lang/msl/writer/raise/raise.cc
index 4699f02..5a0373f 100644
--- a/src/tint/lang/msl/writer/raise/raise.cc
+++ b/src/tint/lang/msl/writer/raise/raise.cc
@@ -160,10 +160,7 @@
RUN_TRANSFORM(raise::BinaryPolyfill, module);
RUN_TRANSFORM(raise::BuiltinPolyfill, module);
- // TODO(crbug.com/419804339): Fix backend compile failures for f16 types related to this
- // transform
- constexpr bool kEnabledModuleConstantTransform = false;
- if (kEnabledModuleConstantTransform) {
+ if (options.enable_module_constant) {
RUN_TRANSFORM(raise::ModuleConstant, module);
}
