Skip to content

Commit

Permalink
[hlsl] Move symbol renaming into the backend
Browse files Browse the repository at this point in the history
Instead of running the AST renamer transform in Dawn's D3D backends,
we just modify the names when emitting them in the printer.

This is a little cleaner than using a transform, as transforms cannot
rename structures and struct members without requiring
const_cast. This also keeps the list of HLSL keywords local to the
printer, and means we can retain the meaningful names throughout the
`raise` process.

Bug: 380043958
Change-Id: Ia654c14eabefe834a438bce841f83d7805602e9a
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/219296
Reviewed-by: Antonio Maiorano <[email protected]>
Commit-Queue: James Price <[email protected]>
  • Loading branch information
jrprice authored and Dawn LUCI CQ committed Dec 13, 2024
1 parent 6c6c6dc commit a53fe68
Show file tree
Hide file tree
Showing 5,826 changed files with 46,341 additions and 45,611 deletions.
The diff you're trying to view is too large. We only load the first 3000 changed files.
21 changes: 13 additions & 8 deletions src/dawn/native/d3d/ShaderUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -218,14 +218,19 @@ MaybeError TranslateToHLSL(d3d::HlslCompilationRequest r,
transformManager.Add<tint::ast::transform::SingleEntryPoint>();
transformInputs.Add<tint::ast::transform::SingleEntryPoint::Config>(r.entryPointName.data());

// Needs to run before all other transforms so that they can use builtin names safely.
tint::ast::transform::Renamer::Remappings requestedNames = {
{std::string(r.entryPointName), kRemappedEntryPointName}};
transformManager.Add<tint::ast::transform::Renamer>();
transformInputs.Add<tint::ast::transform::Renamer::Config>(
r.disableSymbolRenaming ? tint::ast::transform::Renamer::Target::kHlslKeywords
: tint::ast::transform::Renamer::Target::kAll,
std::move(requestedNames));
if (r.useTintIR) {
r.tintOptions.strip_all_names = !r.disableSymbolRenaming;
r.tintOptions.remapped_entry_point_name = kRemappedEntryPointName;
} else {
// Needs to run before all other transforms so that they can use builtin names safely.
tint::ast::transform::Renamer::Remappings requestedNames = {
{std::string(r.entryPointName), kRemappedEntryPointName}};
transformManager.Add<tint::ast::transform::Renamer>();
transformInputs.Add<tint::ast::transform::Renamer::Config>(
r.disableSymbolRenaming ? tint::ast::transform::Renamer::Target::kHlslKeywords
: tint::ast::transform::Renamer::Target::kAll,
std::move(requestedNames));
}

if (r.stage == SingleShaderStage::Vertex) {
transformManager.Add<tint::ast::transform::FirstIndexOffset>();
Expand Down
10 changes: 9 additions & 1 deletion src/tint/cmd/tint/main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,10 @@ When specified, automatically enables HLSL validation)",
}
case Format::kHlsl:
case Format::kHlslFxc: {
if (options.use_ir) {
// Renaming is handled in the backend.
break;
}
if (!options.rename_all) {
transform_inputs.Add<tint::ast::transform::Renamer::Config>(
tint::ast::transform::Renamer::Target::kHlslKeywords);
Expand Down Expand Up @@ -1006,8 +1010,12 @@ bool GenerateHlsl([[maybe_unused]] Options& options,
}

const bool for_fxc = options.format == Format::kHlslFxc;
// TODO(jrprice): Provide a way for the user to set non-default options.
// Set up the backend options.
tint::hlsl::writer::Options gen_options;
if (options.rename_all) {
gen_options.remapped_entry_point_name = "tint_entry_point";
gen_options.strip_all_names = true;
}
gen_options.disable_robustness = !options.enable_robustness;
gen_options.disable_workgroup_init = options.disable_workgroup_init;
gen_options.bindings = tint::hlsl::writer::GenerateBindings(res.Get());
Expand Down
1 change: 1 addition & 0 deletions src/tint/lang/hlsl/writer/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ cc_library(
"switch_test.cc",
"unary_test.cc",
"var_let_test.cc",
"writer_test.cc",
],
deps = [
"//src/tint/api/common",
Expand Down
1 change: 1 addition & 0 deletions src/tint/lang/hlsl/writer/BUILD.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ tint_add_target(tint_lang_hlsl_writer_test test
lang/hlsl/writer/switch_test.cc
lang/hlsl/writer/unary_test.cc
lang/hlsl/writer/var_let_test.cc
lang/hlsl/writer/writer_test.cc
)

tint_target_add_dependencies(tint_lang_hlsl_writer_test test
Expand Down
1 change: 1 addition & 0 deletions src/tint/lang/hlsl/writer/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ if (tint_build_unittests) {
"switch_test.cc",
"unary_test.cc",
"var_let_test.cc",
"writer_test.cc",
]
deps = [
"${dawn_root}/src/utils:utils",
Expand Down
14 changes: 7 additions & 7 deletions src/tint/lang/hlsl/writer/arraylength_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ void foo() {
}

TEST_F(HlslWriterTest, ArrayLength_Robustness) {
auto* dst = b.Var("dst", ty.ptr(storage, ty.array<u32>()));
auto* dst = b.Var("dest", ty.ptr(storage, ty.array<u32>()));
dst->SetBindingPoint(0, 1);
b.ir.root_block->Append(dst);
auto* func = b.Function("foo", ty.void_(), core::ir::Function::PipelineStage::kFragment);
Expand All @@ -243,18 +243,18 @@ TEST_F(HlslWriterTest, ArrayLength_Robustness) {
options.disable_robustness = false;
ASSERT_TRUE(Generate(options)) << err_ << output_.hlsl;
EXPECT_EQ(output_.hlsl, R"(
RWByteAddressBuffer dst : register(u1);
RWByteAddressBuffer dest : register(u1);
void foo() {
uint v = 0u;
dst.GetDimensions(v);
dst.Store((0u + (min(0u, ((v / 4u) - 1u)) * 4u)), 123u);
dest.GetDimensions(v);
dest.Store((0u + (min(0u, ((v / 4u) - 1u)) * 4u)), 123u);
}
)");
}

TEST_F(HlslWriterTest, ArrayLength_RobustnessAndArrayLengthFromUniform) {
auto* dst = b.Var("dst", ty.ptr(storage, ty.array<u32>()));
auto* dst = b.Var("dest", ty.ptr(storage, ty.array<u32>()));
dst->SetBindingPoint(0, 1);
b.ir.root_block->Append(dst);
auto* func = b.Function("foo", ty.void_(), core::ir::Function::PipelineStage::kFragment);
Expand All @@ -270,12 +270,12 @@ TEST_F(HlslWriterTest, ArrayLength_RobustnessAndArrayLengthFromUniform) {
options.array_length_from_uniform.bindpoint_to_size_index[{0, 1}] = 0;
ASSERT_TRUE(Generate(options)) << err_ << output_.hlsl;
EXPECT_EQ(output_.hlsl, R"(
RWByteAddressBuffer dst : register(u1);
RWByteAddressBuffer dest : register(u1);
cbuffer cbuffer_tint_storage_buffer_sizes : register(b0, space30) {
uint4 tint_storage_buffer_sizes[1];
};
void foo() {
dst.Store((0u + (min(0u, ((tint_storage_buffer_sizes[0u].x / 4u) - 1u)) * 4u)), 123u);
dest.Store((0u + (min(0u, ((tint_storage_buffer_sizes[0u].x / 4u) - 1u)) * 4u)), 123u);
}
)");
Expand Down
9 changes: 9 additions & 0 deletions src/tint/lang/hlsl/writer/common/options.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

#include <bitset>
#include <optional>
#include <string>
#include <unordered_map>
#include <vector>

Expand Down Expand Up @@ -199,6 +200,12 @@ struct Options {
/// @returns this Options
Options& operator=(const Options&);

/// An optional remapped name to use when emitting the entry point.
std::optional<std::string> remapped_entry_point_name = {};

/// Set to `true` to strip all user-declared identifiers from the module.
bool strip_all_names = false;

/// Set to `true` to disable software robustness that prevents out-of-bounds accesses.
bool disable_robustness = false;

Expand Down Expand Up @@ -243,6 +250,8 @@ struct Options {

/// Reflect the fields of this class so that it can be used by tint::ForeachField()
TINT_REFLECT(Options,
remapped_entry_point_name,
strip_all_names,
disable_robustness,
disable_workgroup_init,
truncate_interstage_variables,
Expand Down
Loading

0 comments on commit a53fe68

Please sign in to comment.