[spirv-reader] Don't hoist pointers that are already in scope.
Bug: tint:3, tint:213
Change-Id: I79410c670b8690c1f1ea86b7b9427f272a4ebbbb
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/27720
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Commit-Queue: dan sinclair <dsinclair@chromium.org>
diff --git a/src/reader/spirv/function.cc b/src/reader/spirv/function.cc
index 5fb6b2c..724ff47 100644
--- a/src/reader/spirv/function.cc
+++ b/src/reader/spirv/function.cc
@@ -3247,12 +3247,21 @@
// Update the usage span for IDs used by this instruction.
// But skip uses in OpPhi because they are handled differently.
if (inst.opcode() != SpvOpPhi) {
- inst.ForEachInId([this, block_pos](const uint32_t* id_ptr) {
+ inst.ForEachInId([this, block_pos, block_info](const uint32_t* id_ptr) {
auto* def_info = GetDefInfo(*id_ptr);
if (def_info) {
def_info->num_uses++;
def_info->last_use_pos =
std::max(def_info->last_use_pos, block_pos);
+
+ // Determine whether this ID is defined in a different construct
+ // from this use.
+ const auto defining_block = block_order_[def_info->block_pos];
+ const auto* def_in_construct =
+ GetBlockInfo(defining_block)->construct;
+ if (def_in_construct != block_info->construct) {
+ def_info->used_in_another_construct = true;
+ }
}
});
}
@@ -3317,8 +3326,6 @@
const auto first_pos = def_info->block_pos;
const auto last_use_pos = def_info->last_use_pos;
- const auto* const construct_with_last_use =
- GetBlockInfo(block_order_[last_use_pos])->construct;
const auto* def_in_construct =
GetBlockInfo(block_order_[first_pos])->construct;
// A definition in the first block of an kIfSelection or kSwitchSelection
@@ -3331,7 +3338,22 @@
}
}
- if (def_in_construct != construct_with_last_use) {
+ bool should_hoist = false;
+ if (!def_in_construct->ContainsPos(last_use_pos)) {
+ // To satisfy scoping, we have to hoist the definition out to an enclosing
+ // construct.
+ should_hoist = true;
+ } else {
+ // Avoid moving combinatorial values across constructs. This is a
+ // simple heuristic to avoid changing the cost of an operation
+ // by moving it into or out of a loop, for example.
+ if ((def_info->storage_class == ast::StorageClass::kNone) &&
+ def_info->used_in_another_construct) {
+ should_hoist = true;
+ }
+ }
+
+ if (should_hoist) {
const auto* enclosing_construct =
GetEnclosingScope(first_pos, last_use_pos);
if (enclosing_construct == def_in_construct) {
diff --git a/src/reader/spirv/function.h b/src/reader/spirv/function.h
index 6d3ebb3..007c756 100644
--- a/src/reader/spirv/function.h
+++ b/src/reader/spirv/function.h
@@ -223,6 +223,10 @@
/// at all. The "last" ordering is determined by the function block order.
uint32_t last_use_pos = 0;
+ /// Is this value used in a construct other than the one in which it was
+ /// defined?
+ bool used_in_another_construct = false;
+
/// True if this ID requires a WGSL 'const' definition, due to context. It
/// might get one anyway (so this is *not* an if-and-only-if condition).
bool requires_named_const_def = false;
@@ -248,7 +252,7 @@
std::string phi_var;
/// The storage class to use for this value, if it is of pointer type.
- /// This is required to carry a stroage class override from a storage
+ /// This is required to carry a storage class override from a storage
/// buffer expressed in the old style (with Uniform storage class)
/// that needs to be remapped to StorageBuffer storage class.
/// This is kNone for non-pointers.
diff --git a/src/reader/spirv/function_var_test.cc b/src/reader/spirv/function_var_test.cc
index 2867a74..ebbac3d 100644
--- a/src/reader/spirv/function_var_test.cc
+++ b/src/reader/spirv/function_var_test.cc
@@ -728,7 +728,7 @@
EmitStatement_CombinatorialValue_Immediate_UsedOnceDifferentConstruct) {
// Translation should not sink expensive operations into or out of control
// flow. As a simple heuristic, don't move *any* combinatorial operation
- // across any constrol flow.
+ // across any control flow.
auto assembly = Preamble() + R"(
%100 = OpFunction %void None %voidfn
@@ -1182,6 +1182,74 @@
)")) << ToString(fe.ast_body());
}
+TEST_F(SpvParserTest,
+ EmitStatement_CombinatorialNonPointer_Hoisting_DefAndUseFirstBlockIf) {
+ // In this test, both the defintion and the use are in the first block
+ // of an IfSelection. No hoisting occurs because hoisting is triggered
+ // on whether the defining construct contains the last use, rather than
+ // whether the two constructs are the same.
+ //
+ // This example has two SSA IDs which are tempting to hoist but should not:
+ // %1 is defined and used in the first block of an IfSelection.
+ // Do not hoist it.
+ auto assembly = Preamble() + R"(
+ %cond = OpConstantTrue %bool
+
+ %100 = OpFunction %void None %voidfn
+
+ ; in IfSelection construct, nested in Function construct
+ %10 = OpLabel
+ %1 = OpCopyObject %uint %uint_1
+ %2 = OpCopyObject %uint %1
+ OpSelectionMerge %99 None
+ OpBranchConditional %cond %20 %99
+
+ %20 = OpLabel ; in IfSelection construct
+ OpBranch %99
+
+ %99 = OpLabel
+ OpReturn
+
+ OpFunctionEnd
+ )";
+ auto* p = parser(test::Assemble(assembly));
+ ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly;
+ FunctionEmitter fe(p, *spirv_function(100));
+ EXPECT_TRUE(fe.EmitBody()) << p->error();
+
+ // We don't hoist x_1 into its own mutable variable. It is emitted as
+ // a const definition.
+ EXPECT_THAT(ToString(fe.ast_body()), Eq(R"(VariableDeclStatement{
+ Variable{
+ x_1
+ none
+ __u32
+ {
+ ScalarConstructor{1}
+ }
+ }
+}
+VariableDeclStatement{
+ Variable{
+ x_2
+ none
+ __u32
+ {
+ Identifier{x_1}
+ }
+ }
+}
+If{
+ (
+ ScalarConstructor{true}
+ )
+ {
+ }
+}
+Return{}
+)")) << ToString(fe.ast_body());
+}
+
TEST_F(SpvParserTest, EmitStatement_Phi_SingleBlockLoopIndex) {
auto assembly = Preamble() + R"(
%pty = OpTypePointer Private %uint
