Skip to content

Commit 9cf1e48

Browse files
Peter McNeeleyDawn LUCI CQ
authored andcommitted
[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>
1 parent 9104360 commit 9cf1e48

File tree

7 files changed

+22
-5
lines changed

7 files changed

+22
-5
lines changed

src/dawn/native/Toggles.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -570,6 +570,9 @@ static constexpr ToggleEnumAndInfoList kToggleNameAndInfoList = {{
570570
{"disable_polyfills_on_integer_div_and_mod",
571571
"Disable the Tint polyfills on integer division and modulo.", "https://crbug.com/tint/2128",
572572
ToggleStage::Device}},
573+
{Toggle::MetalEnableModuleConstant,
574+
{"metal_enable_module_constant_transform", "Enable the module constant transform.",
575+
"https://crbug.com/419804339", ToggleStage::Device}},
573576
{Toggle::EnableImmediateErrorHandling,
574577
{"enable_immediate_error_handling",
575578
"Have the uncaptured error callback invoked immediately when an error occurs, rather than "

src/dawn/native/Toggles.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ enum class Toggle {
138138
ExposeWGSLTestingFeatures,
139139
ExposeWGSLExperimentalFeatures,
140140
DisablePolyfillsOnIntegerDivisonAndModulo,
141+
MetalEnableModuleConstant,
141142
EnableImmediateErrorHandling,
142143
VulkanUseStorageInputOutput16,
143144
D3D12DontUseShaderModel66OrHigher,

src/dawn/native/metal/PhysicalDeviceMTL.mm

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929

3030
#include "dawn/common/CoreFoundationRef.h"
3131
#include "dawn/common/GPUInfo.h"
32+
#include "dawn/common/GPUInfo_autogen.h"
3233
#include "dawn/common/Log.h"
3334
#include "dawn/common/NSRef.h"
3435
#include "dawn/common/Platform.h"
@@ -441,6 +442,13 @@ bool IsGPUCounterSupported(id<MTLDevice> device,
441442
deviceToggles->Default(Toggle::MetalRenderR8RG8UnormSmallMipToTempTexture, true);
442443
}
443444

445+
// chromium:419804339: Module constant hoisting is broadly available as a msl transform but
446+
// there are execution correction issues with f16 for non apple silicon (Intel/AMD). Therefore
447+
// we only enable for apple silicon for now.
448+
if (gpu_info::IsApple(vendorId)) {
449+
deviceToggles->Default(Toggle::MetalEnableModuleConstant, true);
450+
}
451+
444452
// On some Intel GPUs vertex only render pipeline get wrong depth result if no fragment
445453
// shader provided. Create a placeholder fragment shader module to work around this issue.
446454
if (gpu_info::IsIntel(vendorId)) {

src/dawn/native/metal/ShaderModuleMTL.mm

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,8 @@
298298
req.tintOptions.bindings = std::move(bindings);
299299
req.tintOptions.disable_polyfill_integer_div_mod =
300300
device->IsToggleEnabled(Toggle::DisablePolyfillsOnIntegerDivisonAndModulo);
301+
req.tintOptions.enable_module_constant =
302+
device->IsToggleEnabled(Toggle::MetalEnableModuleConstant);
301303
req.tintOptions.vertex_pulling_config = std::move(vertexPullingTransformConfig);
302304
req.tintOptions.enable_integer_range_analysis =
303305
device->IsToggleEnabled(Toggle::EnableIntegerRangeAnalysisInRobustness);

src/tint/lang/msl/writer/common/options.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,9 @@ struct Options {
163163
/// Set to `true` to disable the polyfills on integer division and modulo.
164164
bool disable_polyfill_integer_div_mod = false;
165165

166+
/// Set to `true` to enable the module constant transform
167+
bool enable_module_constant = false;
168+
166169
/// Emit argument buffers
167170
bool use_argument_buffers = false;
168171

@@ -197,6 +200,7 @@ struct Options {
197200
disable_demote_to_helper,
198201
emit_vertex_point_size,
199202
disable_polyfill_integer_div_mod,
203+
enable_module_constant,
200204
use_argument_buffers,
201205
buffer_size_ubo_index,
202206
fixed_sample_mask,

src/tint/lang/msl/writer/printer/printer.cc

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -662,7 +662,9 @@ class Printer : public tint::TextGenerator {
662662

663663
if (current_function_ == nullptr) {
664664
// program scope let
665-
out << "constexpr constant ";
665+
// TODO(crbug.com/419804339): This should be 'constexpr constant' but the matrix class
666+
// (constructor) in metal is not constexpr.
667+
out << "const constant ";
666668
}
667669
EmitType(out, l->Result()->Type());
668670
out << " ";

src/tint/lang/msl/writer/raise/raise.cc

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -160,10 +160,7 @@ Result<RaiseResult> Raise(core::ir::Module& module, const Options& options) {
160160
RUN_TRANSFORM(raise::BinaryPolyfill, module);
161161
RUN_TRANSFORM(raise::BuiltinPolyfill, module);
162162

163-
// TODO(crbug.com/419804339): Fix backend compile failures for f16 types related to this
164-
// transform
165-
constexpr bool kEnabledModuleConstantTransform = false;
166-
if (kEnabledModuleConstantTransform) {
163+
if (options.enable_module_constant) {
167164
RUN_TRANSFORM(raise::ModuleConstant, module);
168165
}
169166

0 commit comments

Comments
 (0)