encounter/dawn-cmake
1
0
Fork 0
mirror of https://github.com/encounter/dawn-cmake.git synced 2025-12-10 05:57:51 +00:00
Code Issues Packages Projects Releases Wiki Activity

spirv-reader: phis as a particular case of hoisting to a var

Browse Source 
We already compute the "first" and "last" basic block that
uses a value, so we could know when to hoist a value into
a var declaration. You have to do this sometimes to make
sure all uses are in scope of the declaration.

Until now we tracked Phis with an entirely different mechanism.
But there are cases which broke down. That's what happens
in crbug.com/tint/1649.

Additionally, GraphicsFuzz cases generarte similar weirdness.

Also, be more careful about ensuring that the assignments
generated to feed phis behave as if they occur in parallel.
Within a single batch of such assignments, generate and
use intermediate let-declarations for phis that that batch
will overwrite.

Also, unwrap-references when rectifying the signedness of
binary operators.

Skip tests that fail due to crbug.comt/tint/98:
  test/tint/bug/tint/749.spvasm.*

Fixed: tint:1649
Change-Id: I7314c351b74a10bfa9a18011f3d80a520568011c
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/101220
Auto-Submit: David Neto <dneto@google.com>
Commit-Queue: David Neto <dneto@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
This commit is contained in:
David Neto
2022-09-16 20:18:39 +00:00
committed by Dawn LUCI CQ
parent ebc5bba671
commit a3f2bf6c60
23 changed files with 889 additions and 11014 deletions
Download Patch File Download Diff File Expand all files Collapse all files

186
src/tint/reader/spirv/function.cc
View File

@@ -37,6 +37,8 @@
#include "src/tint/sem/depth_texture.h"
#include "src/tint/sem/sampled_texture.h"
#include "src/tint/transform/spirv_atomic.h"
#include "src/tint/utils/hashmap.h"
#include "src/tint/utils/hashset.h"
// Terms:
// CFG: the control flow graph of the function, where basic blocks are the
@@ -3356,16 +3358,6 @@ bool FunctionEmitter::EmitStatementsInBasicBlock(const BlockInfo& block_info,
auto* type = ty_.Reference(storage_type, ast::StorageClass::kNone);
identifier_types_.emplace(id, type);
}
// Emit declarations of phi state variables, in index order.
for (auto id : sorted_by_index(block_info.phis_needing_state_vars)) {
const auto* def_inst = def_use_mgr_->GetDef(id);
TINT_ASSERT(Reader, def_inst);
const auto phi_var_name = GetDefInfo(id)->phi_var;
TINT_ASSERT(Reader, !phi_var_name.empty());
auto* var = builder_.Var(phi_var_name,
parser_impl_.ConvertType(def_inst->type_id())->Build(builder_));
AddStatement(create<ast::VariableDeclStatement>(Source{}, var));
}
// Emit regular statements.
const spvtools::opt::BasicBlock& bb = *(block_info.basic_block);
@@ -3384,22 +3376,55 @@ bool FunctionEmitter::EmitStatementsInBasicBlock(const BlockInfo& block_info,
// Emit assignments to carry values to phi nodes in potential destinations.
// Do it in index order.
if (!block_info.phi_assignments.IsEmpty()) {
auto sorted = block_info.phi_assignments;
// Keep only the phis that are used.
utils::Vector<BlockInfo::PhiAssignment, 4> worklist;
worklist.Reserve(block_info.phi_assignments.Length());
for (const auto assignment : block_info.phi_assignments) {
if (GetDefInfo(assignment.phi_id)->num_uses > 0) {
worklist.Push(assignment);
}
}
// Sort them.
std::stable_sort(
sorted.begin(), sorted.end(),
worklist.begin(), worklist.end(),
[this](const BlockInfo::PhiAssignment& lhs, const BlockInfo::PhiAssignment& rhs) {
return GetDefInfo(lhs.phi_id)->index < GetDefInfo(rhs.phi_id)->index;
});
for (auto assignment : block_info.phi_assignments) {
const auto var_name = GetDefInfo(assignment.phi_id)->phi_var;
auto expr = MakeExpression(assignment.value);
if (!expr) {
return false;
// Generate assignments to the phi variables being fed by this
// block. It must act as a parallel assignment. So first capture the
// current value of any value that will be overwritten, then generate
// the assignments.
// The set of IDs that are read by the assignments.
utils::Hashset<uint32_t, 8> read_set;
for (const auto assignment : worklist) {
read_set.Add(assignment.value_id);
}
// Generate a let-declaration to capture the current value of each phi
// that will be both read and written.
utils::Hashmap<uint32_t, Symbol, 8> copied_phis;
for (const auto assignment : worklist) {
const auto phi_id = assignment.phi_id;
if (read_set.Find(phi_id)) {
auto copy_name = namer_.MakeDerivedName(namer_.Name(phi_id) + "_c" +
std::to_string(block_info.id));
auto copy_sym = builder_.Symbols().Register(copy_name);
copied_phis.GetOrCreate(phi_id, [copy_sym]() { return copy_sym; });
AddStatement(builder_.WrapInStatement(
builder_.Let(copy_sym, builder_.Expr(namer_.Name(phi_id)))));
}
AddStatement(create<ast::AssignmentStatement>(
Source{},
create<ast::IdentifierExpression>(Source{}, builder_.Symbols().Register(var_name)),
expr.expr));
}
// Generate assignments to the phi vars.
for (const auto assignment : worklist) {
const auto phi_id = assignment.phi_id;
auto* const lhs_expr = builder_.Expr(namer_.Name(phi_id));
// If RHS value is actually a phi we just cpatured, then use it.
auto* const copy_sym = copied_phis.Find(assignment.value_id);
auto* const rhs_expr =
copy_sym ? builder_.Expr(*copy_sym) : MakeExpression(assignment.value_id).expr;
AddStatement(builder_.Assign(lhs_expr, rhs_expr));
}
}
@@ -3692,11 +3717,8 @@ bool FunctionEmitter::EmitStatement(const spvtools::opt::Instruction& inst) {
}
case SpvOpPhi: {
// Emit a read from the associated state variable.
TypedExpression expr{parser_impl_.ConvertType(inst.type_id()),
create<ast::IdentifierExpression>(
Source{}, builder_.Symbols().Register(def_info->phi_var))};
return EmitConstDefOrWriteToHoistedVar(inst, expr);
// The value will be in scope, available for reading from the phi ID.
return true;
}
case SpvOpOuterProduct:
@@ -4884,60 +4906,80 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() {
}
}
// Scan uses of locally defined IDs, in function block order.
// Scan uses of locally defined IDs, finding their first and last uses, in
// block order.
// Updates the span of block positions that this value is used in.
// Ignores values defined outside this function.
auto record_value_use = [this](uint32_t id, const BlockInfo* block_info) {
if (auto* def_info = GetDefInfo(id)) {
// Update usage count.
def_info->num_uses++;
// Update usage span.
def_info->first_use_pos = std::min(def_info->first_use_pos, block_info->pos);
def_info->last_use_pos = std::max(def_info->last_use_pos, block_info->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;
}
}
};
for (auto block_id : block_order_) {
const auto* block_info = GetBlockInfo(block_id);
const auto block_pos = block_info->pos;
for (const auto& inst : *(block_info->basic_block)) {
// Update bookkeeping for locally-defined IDs used by this instruction.
inst.ForEachInId([this, block_pos, block_info](const uint32_t* id_ptr) {
auto* def_info = GetDefInfo(*id_ptr);
if (def_info) {
// Update usage count.
def_info->num_uses++;
// Update usage span.
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;
}
}
});
if (inst.opcode() == SpvOpPhi) {
// Declare a name for the variable used to carry values to a phi.
// For an OpPhi defining value P, an incoming value V from parent block B is
// counted as being "used" at block B, not at the block containing the Phi.
// That's because we will create a variable PHI_P to hold the phi value, and
// in the code generated for block B, create assignment `PHI_P = V`.
// To make the WGSL scopes work, both P and V are counted as being "used"
// in the parent block B.
const auto phi_id = inst.result_id();
auto* phi_def_info = GetDefInfo(phi_id);
phi_def_info->phi_var = namer_.MakeDerivedName(namer_.Name(phi_id) + "_phi");
phi_def_info->is_phi = true;
// Track all the places where we need to mention the variable,
// so we can place its declaration. First, record the location of
// the read from the variable.
uint32_t first_pos = block_pos;
uint32_t last_pos = block_pos;
// Record the assignments that will propagate values from predecessor
// blocks.
for (uint32_t i = 0; i + 1 < inst.NumInOperands(); i += 2) {
const uint32_t value_id = inst.GetSingleWordInOperand(i);
const uint32_t incoming_value_id = inst.GetSingleWordInOperand(i);
const uint32_t pred_block_id = inst.GetSingleWordInOperand(i + 1);
auto* pred_block_info = GetBlockInfo(pred_block_id);
// The predecessor might not be in the block order at all, so we
// need this guard.
if (IsInBlockOrder(pred_block_info)) {
// Track where the incoming value needs to be in scope.
record_value_use(incoming_value_id, block_info);
// Track where P needs to be in scope. It's not an ordinary use, so don't
// count it as one.
const auto pred_pos = pred_block_info->pos;
phi_def_info->first_use_pos =
std::min(phi_def_info->first_use_pos, pred_pos);
phi_def_info->last_use_pos = std::max(phi_def_info->last_use_pos, pred_pos);
// Record the assignment that needs to occur at the end
// of the predecessor block.
pred_block_info->phi_assignments.Push({phi_id, value_id});
first_pos = std::min(first_pos, pred_block_info->pos);
last_pos = std::max(last_pos, pred_block_info->pos);
pred_block_info->phi_assignments.Push({phi_id, incoming_value_id});
}
}
// Schedule the declaration of the state variable.
const auto* enclosing_construct = GetEnclosingScope(first_pos, last_pos);
const auto* enclosing_construct =
GetEnclosingScope(phi_def_info->first_use_pos, phi_def_info->last_use_pos);
GetBlockInfo(enclosing_construct->begin_id)->phis_needing_state_vars.Push(phi_id);
} else {
inst.ForEachInId([block_info, &record_value_use](const uint32_t* id_ptr) {
record_value_use(*id_ptr, block_info);
});
}
}
}
@@ -4967,41 +5009,47 @@ void FunctionEmitter::FindValuesNeedingNamedOrHoistedDefinition() {
continue;
}
// The first use must be the at the SSA definition, because block order
// respects dominance.
const auto first_pos = def_info->block_pos;
const auto last_use_pos = def_info->last_use_pos;
const auto* def_in_construct = GetBlockInfo(block_order_[first_pos])->construct;
const auto* def_in_construct = GetBlockInfo(block_order_[def_info->block_pos])->construct;
// A definition in the first block of an kIfSelection or kSwitchSelection
// occurs before the branch, and so that definition should count as
// having been defined at the scope of the parent construct.
if (first_pos == def_in_construct->begin_pos) {
if (def_info->block_pos == def_in_construct->begin_pos) {
if ((def_in_construct->kind == Construct::kIfSelection) ||
(def_in_construct->kind == Construct::kSwitchSelection)) {
def_in_construct = def_in_construct->parent;
}
}
bool should_hoist = false;
if (!def_in_construct->ContainsPos(last_use_pos)) {
// We care about the earliest between the place of definition, and the first
// use of the value.
const auto first_pos = std::min(def_info->block_pos, def_info->first_use_pos);
const auto last_use_pos = def_info->last_use_pos;
bool should_hoist_to_let = false;
bool should_hoist_to_var = false;
if (def_info->is_phi) {
// We need to generate a variable, and assignments to that variable in
// all the phi parent blocks.
should_hoist_to_var = true;
} else if (!def_in_construct->ContainsPos(first_pos) ||
!def_in_construct->ContainsPos(last_use_pos)) {
// To satisfy scoping, we have to hoist the definition out to an enclosing
// construct.
should_hoist = true;
should_hoist_to_var = 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::kInvalid) &&
def_info->used_in_another_construct) {
should_hoist = true;
should_hoist_to_let = true;
}
}
if (should_hoist) {
if (should_hoist_to_var || should_hoist_to_let) {
const auto* enclosing_construct = GetEnclosingScope(first_pos, last_use_pos);
if (enclosing_construct == def_in_construct) {
// We can use a plain 'const' definition.
if (should_hoist_to_let && (enclosing_construct == def_in_construct)) {
// We can use a plain 'let' declaration.
def_info->requires_named_const_def = true;
} else {
// We need to make a hoisted variable definition.

39
src/tint/reader/spirv/function.h
View File

@@ -15,6 +15,7 @@
#ifndef SRC_TINT_READER_SPIRV_FUNCTION_H_
#define SRC_TINT_READER_SPIRV_FUNCTION_H_
#include <limits>
#include <memory>
#include <optional>
#include <string>
@@ -166,8 +167,8 @@ struct BlockInfo {
struct PhiAssignment {
/// The ID of an OpPhi receiving a value from this basic block.
uint32_t phi_id;
/// The the value carried to the given OpPhi.
uint32_t value;
/// The ID of the value carried to the given OpPhi.
uint32_t value_id;
};
/// If this basic block branches to a visited basic block containing phis,
/// then this is the list of writes to the variables associated those phis.
@@ -258,10 +259,9 @@ struct DefInfo {
/// True if the definition of this ID is inside the function.
const bool locally_defined = true;
/// The position of the first block in which this ID is visible, in function
/// block order. For IDs defined outside of the function, it is 0.
/// For IDs defined in the function, it is the position of the block
/// containing the definition of the ID.
/// For IDs defined in the function, this is the position of the block
/// containing the definition of the ID, in function block order.
/// For IDs defined outside of the function, it is 0.
/// See method `FunctionEmitter::ComputeBlockOrderAndPositions`
const uint32_t block_pos = 0;
@@ -272,8 +272,17 @@ struct DefInfo {
/// The number of uses of this ID.
uint32_t num_uses = 0;
/// The block position of the first use of this ID, or MAX_UINT if it is not
/// used at all. The "first" ordering is determined by the function block
/// order. The first use of an ID might be in an OpPhi that precedes the
/// definition of the ID.
/// The ID defined by an OpPhi is counted as being "used" in each of its
/// parent blocks.
uint32_t first_use_pos = std::numeric_limits<uint32_t>::max();
/// The block position of the last use of this ID, or 0 if it is not used
/// at all. The "last" ordering is determined by the function block order.
/// The ID defined by an OpPhi is counted as being "used" in each of its
/// parent blocks.
uint32_t last_use_pos = 0;
/// Is this value used in a construct other than the one in which it was
@@ -288,8 +297,8 @@ struct DefInfo {
/// corresponding position of the ID definition in SPIR-V. This compensates
/// for the difference between dominance and scoping. An SSA definition can
/// dominate all its uses, but the construct where it is defined does not
/// enclose all the uses, and so if it were declared as a WGSL constant
/// definition at the point of its SPIR-V definition, then the WGSL name
/// enclose all the uses, and so if it were declared as a WGSL let-
/// declaration at the point of its SPIR-V definition, then the WGSL name
/// would go out of scope too early. Fix that by creating a variable at the
/// top of the smallest construct that encloses both the definition and all
/// its uses. Then the original SPIR-V definition maps to a WGSL assignment
@@ -299,10 +308,8 @@ struct DefInfo {
/// example, pointers. crbug.com/tint/98
bool requires_hoisted_def = false;
/// If the definition is an OpPhi, then `phi_var` is the name of the
/// variable that stores the value carried from parent basic blocks into
/// the basic block containing the OpPhi. Otherwise this is the empty string.
std::string phi_var;
/// Is this ID an OpPhi?
bool is_phi = false;
/// The storage class to use for this value, if it is of pointer type.
/// This is required to carry a storage class override from a storage
@@ -332,11 +339,11 @@ inline std::ostream& operator<<(std::ostream& o, const DefInfo& di) {
<< " inst.result_id: " << di.inst.result_id()
<< " locally_defined: " << (di.locally_defined ? "true" : "false")
<< " block_pos: " << di.block_pos << " num_uses: " << di.num_uses
<< " last_use_pos: " << di.last_use_pos
<< " first_use_pos: " << di.first_use_pos << " last_use_pos: " << di.last_use_pos
<< " used_in_another_construct: " << (di.used_in_another_construct ? "true" : "false")
<< " requires_named_const_def: " << (di.requires_named_const_def ? "true" : "false")
<< " requires_hoisted_def: " << (di.requires_hoisted_def ? "true" : "false") << " phi_var: '"
<< di.phi_var << "'";
<< " requires_hoisted_def: " << (di.requires_hoisted_def ? "true" : "false")
<< " is_phi: " << (di.is_phi ? "true" : "false") << "";
if (di.storage_class != ast::StorageClass::kNone) {
o << " sc:" << int(di.storage_class);
}
@@ -603,7 +610,7 @@ class FunctionEmitter {
/// @returns an possibly updated type
const Type* RemapStorageClass(const Type* type, uint32_t result_id);
/// Marks locally defined values when they should get a 'const'
/// Marks locally defined values when they should get a 'let'
/// definition in WGSL, or a 'var' definition at an outer scope.
/// This occurs in several cases:
/// - When a SPIR-V instruction might use the dynamically computed value

452
src/tint/reader/spirv/function_var_test.cc
View File

@@ -922,6 +922,52 @@ return;
EXPECT_EQ(expect, got);
}
TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_SimultaneousAssignment) {
// Phis must act as if they are simutaneously assigned.
// %101 and %102 should exchange values on each iteration, and never have
// the same value.
auto assembly = Preamble() + R"(
%100 = OpFunction %void None %voidfn
%10 = OpLabel
OpBranch %20
%20 = OpLabel
%101 = OpPhi %bool %true %10 %102 %20
%102 = OpPhi %bool %false %10 %101 %20
OpLoopMerge %99 %20 None
OpBranchConditional %true %99 %20
%99 = OpLabel
OpReturn
OpFunctionEnd
)";
auto p = parser(test::Assemble(assembly));
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly;
auto fe = p->function_emitter(100);
EXPECT_TRUE(fe.EmitBody()) << p->error();
auto ast_body = fe.ast_body();
auto got = test::ToString(p->program(), ast_body);
auto* expect = R"(var x_101 : bool;
var x_102 : bool;
x_101 = true;
x_102 = false;
loop {
let x_101_c20 = x_101;
let x_102_c20 = x_102;
x_101 = x_102_c20;
x_102 = x_101_c20;
if (true) {
break;
}
}
return;
)";
EXPECT_EQ(expect, got);
}
TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_SingleBlockLoopIndex) {
auto assembly = Preamble() + R"(
%pty = OpTypePointer Private %uint
@@ -969,20 +1015,19 @@ TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_SingleBlockLoopIndex) {
auto ast_body = fe.ast_body();
auto got = test::ToString(p->program(), ast_body);
auto* expect = R"(loop {
var x_2_phi : u32;
var x_3_phi : u32;
var x_2 : u32;
var x_3 : u32;
let x_101 : bool = x_7;
let x_102 : bool = x_8;
x_2_phi = 0u;
x_3_phi = 1u;
x_2 = 0u;
x_3 = 1u;
if (x_101) {
break;
}
loop {
let x_2 : u32 = x_2_phi;
let x_3 : u32 = x_3_phi;
x_2_phi = (x_2 + 1u);
x_3_phi = x_3;
let x_3_c20 = x_3;
x_2 = (x_2 + 1u);
x_3 = x_3_c20;
if (x_102) {
break;
}
@@ -1043,27 +1088,26 @@ TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_MultiBlockLoopIndex) {
auto ast_body = fe.ast_body();
auto got = test::ToString(p->program(), ast_body);
auto* expect = R"(loop {
var x_2_phi : u32;
var x_3_phi : u32;
var x_2 : u32;
var x_3 : u32;
let x_101 : bool = x_7;
let x_102 : bool = x_8;
x_2_phi = 0u;
x_3_phi = 1u;
x_2 = 0u;
x_3 = 1u;
if (x_101) {
break;
}
loop {
var x_4 : u32;
let x_2 : u32 = x_2_phi;
let x_3 : u32 = x_3_phi;
if (x_102) {
break;
}
continuing {
x_4 = (x_2 + 1u);
x_2_phi = x_4;
x_3_phi = x_3;
let x_3_c30 = x_3;
x_2 = x_4;
x_3 = x_3_c30;
}
}
}
@@ -1101,6 +1145,7 @@ TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_ValueFromLoopBodyAndContinuin
%30 = OpLabel
%7 = OpIAdd %uint %4 %6 ; use %4 again
%8 = OpCopyObject %uint %5 ; use %5
OpBranch %20
%79 = OpLabel
@@ -1123,24 +1168,25 @@ TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_ValueFromLoopBodyAndContinuin
auto got = test::ToString(p->program(), ast_body);
auto* expect = R"(let x_101 : bool = x_17;
loop {
var x_2_phi : u32;
var x_5_phi : u32;
x_2_phi = 0u;
x_5_phi = 1u;
var x_2 : u32;
var x_5 : u32;
x_2 = 0u;
x_5 = 1u;
loop {
var x_4 : u32;
var x_6 : u32;
var x_7 : u32;
let x_2 : u32 = x_2_phi;
let x_5 : u32 = x_5_phi;
let x_4 : u32 = (x_2 + 1u);
let x_6 : u32 = (x_4 + 1u);
x_4 = (x_2 + 1u);
x_6 = (x_4 + 1u);
if (x_101) {
break;
}
continuing {
x_7 = (x_4 + x_6);
x_2_phi = x_4;
x_5_phi = x_7;
let x_8 : u32 = x_5;
x_2 = x_4;
x_5 = x_7;
}
}
}
@@ -1203,21 +1249,20 @@ TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_FromElseAndThen) {
auto* expect = R"(let x_101 : bool = x_7;
let x_102 : bool = x_8;
loop {
var x_2_phi : u32;
var x_2 : u32;
if (x_101) {
break;
}
if (x_102) {
x_2_phi = 0u;
x_2 = 0u;
continue;
} else {
x_2_phi = 1u;
x_2 = 1u;
continue;
}
x_2_phi = 0u;
x_2 = 0u;
continuing {
let x_2 : u32 = x_2_phi;
x_1 = x_2;
}
}
@@ -1277,13 +1322,13 @@ TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_FromHeaderAndThen) {
auto* expect = R"(let x_101 : bool = x_7;
let x_102 : bool = x_8;
loop {
var x_2_phi : u32;
var x_2 : u32;
if (x_101) {
break;
}
x_2_phi = 0u;
x_2 = 0u;
if (x_102) {
x_2_phi = 1u;
x_2 = 1u;
continue;
} else {
continue;
@@ -1291,7 +1336,6 @@ loop {
return;
continuing {
let x_2 : u32 = x_2_phi;
x_1 = x_2;
}
}
@@ -1334,7 +1378,8 @@ TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_InMerge_PredecessorsDominatdB
%99 = OpLabel
; predecessors are all dominated by case construct head at %30
%phi = OpPhi %uint %uint_0 %45 %uint_1 %50
%41 = OpPhi %uint %uint_0 %45 %uint_1 %50
%101 = OpCopyObject %uint %41 ; give it a use so it's emitted
OpReturn
OpFunctionEnd
@@ -1346,7 +1391,7 @@ TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_InMerge_PredecessorsDominatdB
auto ast_body = fe.ast_body();
auto got = test::ToString(p->program(), ast_body);
auto* expect = R"(var x_41_phi : u32;
auto* expect = R"(var x_41 : u32;
switch(1u) {
default: {
fallthrough;
@@ -1357,19 +1402,19 @@ switch(1u) {
case 1u: {
if (true) {
} else {
x_41_phi = 0u;
x_41 = 0u;
break;
}
x_41_phi = 1u;
x_41 = 1u;
}
}
let x_41 : u32 = x_41_phi;
let x_101 : u32 = x_41;
return;
)";
EXPECT_EQ(expect, got) << got << assembly;
}
TEST_F(SpvParserFunctionVarTest, EmitStatement_UseInPhiCountsAsUse) {
TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_UseInPhiCountsAsUse) {
// From crbug.com/215
// If the only use of a combinatorially computed ID is as the value
// in an OpPhi, then we still have to emit it. The algorithm fix
@@ -1393,6 +1438,7 @@ TEST_F(SpvParserFunctionVarTest, EmitStatement_UseInPhiCountsAsUse) {
%99 = OpLabel
%101 = OpPhi %bool %11 %10 %12 %20
%102 = OpCopyObject %bool %101 ;; ensure a use of %101
OpReturn
OpFunctionEnd
@@ -1405,14 +1451,330 @@ TEST_F(SpvParserFunctionVarTest, EmitStatement_UseInPhiCountsAsUse) {
auto ast_body = fe.ast_body();
auto got = test::ToString(p->program(), ast_body);
auto* expect = R"(var x_101_phi : bool;
auto* expect = R"(var x_101 : bool;
let x_11 : bool = (true & true);
let x_12 : bool = !(x_11);
x_101_phi = x_11;
x_101 = x_11;
if (true) {
x_101_phi = x_12;
x_101 = x_12;
}
let x_101 : bool = x_101_phi;
let x_102 : bool = x_101;
return;
)";
EXPECT_EQ(expect, got);
}
TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_PhiInLoopHeader_FedByHoistedVar_PhiUnused) {
// From investigation into crbug.com/1649
//
// Value %999 is defined deep in control flow, then we arrange for
// it to dominate the backedge of the outer loop. The %999 value is then
// fed back into the phi in the loop header. So %999 needs to be hoisted
// out of the loop. The phi assignment needs to use the hoisted variable.
// The hoisted variable needs to be placed such that its scope encloses
// that phi in the header of the outer loop. The compiler needs
// to "see" that there is an implicit use of %999 in the backedge block
// of that outer loop.
auto assembly = Preamble() + R"(
%100 = OpFunction %void None %voidfn
%10 = OpLabel
OpBranch %20
%20 = OpLabel
%101 = OpPhi %bool %true %10 %999 %80
OpLoopMerge %99 %80 None
OpBranchConditional %true %30 %99
%30 = OpLabel
OpSelectionMerge %50 None
OpBranchConditional %true %40 %50
%40 = OpLabel
%999 = OpCopyObject %bool %true
OpBranch %60
%50 = OpLabel
OpReturn
%60 = OpLabel ; if merge
OpBranch %80
%80 = OpLabel ; continue target
OpBranch %20
%99 = OpLabel
OpReturn
OpFunctionEnd
)";
auto p = parser(test::Assemble(assembly));
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly;
auto fe = p->function_emitter(100);
EXPECT_TRUE(fe.EmitBody()) << p->error();
auto ast_body = fe.ast_body();
auto got = test::ToString(p->program(), ast_body);
auto* expect = R"(loop {
var x_999 : bool;
if (true) {
} else {
break;
}
if (true) {
x_999 = true;
continue;
}
return;
}
return;
)";
EXPECT_EQ(expect, got);
}
TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_PhiInLoopHeader_FedByHoistedVar_PhiUsed) {
// From investigation into crbug.com/1649
//
// Value %999 is defined deep in control flow, then we arrange for
// it to dominate the backedge of the outer loop. The %999 value is then
// fed back into the phi in the loop header. So %999 needs to be hoisted
// out of the loop. The phi assignment needs to use the hoisted variable.
// The hoisted variable needs to be placed such that its scope encloses
// that phi in the header of the outer loop. The compiler needs
// to "see" that there is an implicit use of %999 in the backedge block
// of that outer loop.
auto assembly = Preamble() + R"(
%100 = OpFunction %void None %voidfn
%10 = OpLabel
OpBranch %20
%20 = OpLabel
%101 = OpPhi %bool %true %10 %999 %80
OpLoopMerge %99 %80 None
OpBranchConditional %true %30 %99
%30 = OpLabel
OpSelectionMerge %50 None
OpBranchConditional %true %40 %50
%40 = OpLabel
%999 = OpCopyObject %bool %true
OpBranch %60
%50 = OpLabel
OpReturn
%60 = OpLabel ; if merge
OpBranch %80
%80 = OpLabel ; continue target
OpBranch %20
%99 = OpLabel
%1000 = OpCopyObject %bool %101
OpReturn
OpFunctionEnd
)";
auto p = parser(test::Assemble(assembly));
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly;
auto fe = p->function_emitter(100);
EXPECT_TRUE(fe.EmitBody()) << p->error();
auto ast_body = fe.ast_body();
auto got = test::ToString(p->program(), ast_body);
auto* expect = R"(var x_101 : bool;
x_101 = true;
loop {
var x_999 : bool;
if (true) {
} else {
break;
}
if (true) {
x_999 = true;
continue;
}
return;
continuing {
x_101 = x_999;
}
}
let x_1000 : bool = x_101;
return;
)";
EXPECT_EQ(expect, got);
}
TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_PhiInLoopHeader_FedByPhi_PhiUnused) {
// From investigation into crbug.com/1649
//
// This is a reduction of one of the hard parts of test case
// vk-gl-cts/graphicsfuzz/stable-binarysearch-tree-false-if-discard-loop/1.spvasm
// In particular, see the data flow around %114 in that case.
//
// Here value %999 is is a *phi* defined deep in control flow, then we
// arrange for it to dominate the backedge of the outer loop. The %999
// value is then fed back into the phi in the loop header. The variable
// generated to hold the %999 value needs to be placed such that its scope
// encloses that phi in the header of the outer loop. The compiler needs
// to "see" that there is an implicit use of %999 in the backedge block
// of that outer loop.
auto assembly = Preamble() + R"(
%100 = OpFunction %void None %voidfn
%10 = OpLabel
OpBranch %20
%20 = OpLabel
%101 = OpPhi %bool %true %10 %999 %80
OpLoopMerge %99 %80 None
OpBranchConditional %true %99 %30
%30 = OpLabel
OpLoopMerge %70 %60 None
OpBranch %40
%40 = OpLabel
OpBranchConditional %true %60 %50
%50 = OpLabel
OpBranch %60
%60 = OpLabel ; inner continue
%999 = OpPhi %bool %true %40 %false %50
OpBranchConditional %true %70 %30
%70 = OpLabel ; inner merge
OpBranch %80
%80 = OpLabel ; outer continue target
OpBranch %20
%99 = OpLabel
OpReturn
OpFunctionEnd
)";
auto p = parser(test::Assemble(assembly));
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly;
auto fe = p->function_emitter(100);
EXPECT_TRUE(fe.EmitBody()) << p->error();
auto ast_body = fe.ast_body();
auto got = test::ToString(p->program(), ast_body);
auto* expect = R"(loop {
var x_999 : bool;
if (true) {
break;
}
loop {
x_999 = true;
if (true) {
continue;
}
x_999 = false;
continuing {
if (true) {
break;
}
}
}
}
return;
)";
EXPECT_EQ(expect, got);
}
TEST_F(SpvParserFunctionVarTest, EmitStatement_Phi_PhiInLoopHeader_FedByPhi_PhiUsed) {
// From investigation into crbug.com/1649
//
// This is a reduction of one of the hard parts of test case
// vk-gl-cts/graphicsfuzz/stable-binarysearch-tree-false-if-discard-loop/1.spvasm
// In particular, see the data flow around %114 in that case.
//
// Here value %999 is is a *phi* defined deep in control flow, then we
// arrange for it to dominate the backedge of the outer loop. The %999
// value is then fed back into the phi in the loop header. The variable
// generated to hold the %999 value needs to be placed such that its scope
// encloses that phi in the header of the outer loop. The compiler needs
// to "see" that there is an implicit use of %999 in the backedge block
// of that outer loop.
auto assembly = Preamble() + R"(
%100 = OpFunction %void None %voidfn
%10 = OpLabel
OpBranch %20
%20 = OpLabel
%101 = OpPhi %bool %true %10 %999 %80
OpLoopMerge %99 %80 None
OpBranchConditional %true %99 %30
%30 = OpLabel
OpLoopMerge %70 %60 None
OpBranch %40
%40 = OpLabel
OpBranchConditional %true %60 %50
%50 = OpLabel
OpBranch %60
%60 = OpLabel ; inner continue
%999 = OpPhi %bool %true %40 %false %50
OpBranchConditional %true %70 %30
%70 = OpLabel ; inner merge
OpBranch %80
%80 = OpLabel ; outer continue target
OpBranch %20
%99 = OpLabel
%1000 = OpCopyObject %bool %101
OpReturn
OpFunctionEnd
)";
auto p = parser(test::Assemble(assembly));
ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << assembly;
auto fe = p->function_emitter(100);
EXPECT_TRUE(fe.EmitBody()) << p->error();
auto ast_body = fe.ast_body();
auto got = test::ToString(p->program(), ast_body);
auto* expect = R"(var x_101 : bool;
x_101 = true;
loop {
var x_999 : bool;
if (true) {
break;
}
loop {
x_999 = true;
if (true) {
continue;
}
x_999 = false;
continuing {
if (true) {
break;
}
}
}
continuing {
x_101 = x_999;
}
}
let x_1000 : bool = x_101;
return;
)";
EXPECT_EQ(expect, got);

12
src/tint/reader/spirv/parser_impl.cc
View File

@@ -2053,7 +2053,8 @@ TypedExpression ParserImpl::RectifyOperandSignedness(const spvtools::opt::Instru
Fail() << "internal error: RectifyOperandSignedness given a null expr\n";
return {};
}
auto* type = expr.type;
// TODO(crbug.com/tint/1669) should this unpack aliases too?
auto* type = expr.type->UnwrapRef();
if (!type) {
Fail() << "internal error: unmapped type for: " << expr.expr->TypeInfo().name << "\n";
return {};
@@ -2078,12 +2079,12 @@ TypedExpression ParserImpl::RectifyOperandSignedness(const spvtools::opt::Instru
TypedExpression ParserImpl::RectifySecondOperandSignedness(const spvtools::opt::Instruction& inst,
const Type* first_operand_type,
TypedExpression&& second_operand_expr) {
if ((first_operand_type != second_operand_expr.type) &&
const Type* target_type = first_operand_type->UnwrapRef();
if ((target_type != second_operand_expr.type->UnwrapRef()) &&
AssumesSecondOperandSignednessMatchesFirstOperand(inst.opcode())) {
// Conversion is required.
return {first_operand_type,
create<ast::BitcastExpression>(Source{}, first_operand_type->Build(builder_),
second_operand_expr.expr)};
return {target_type, create<ast::BitcastExpression>(Source{}, target_type->Build(builder_),
second_operand_expr.expr)};
}
// No conversion necessary.
return std::move(second_operand_expr);
@@ -2091,6 +2092,7 @@ TypedExpression ParserImpl::RectifySecondOperandSignedness(const spvtools::opt::
const Type* ParserImpl::ForcedResultType(const spvtools::opt::Instruction& inst,
const Type* first_operand_type) {
first_operand_type = first_operand_type->UnwrapRef();
const auto opcode = inst.opcode();
if (AssumesResultSignednessMatchesFirstOperand(opcode)) {
return first_operand_type;

27
src/tint/reader/spirv/parser_impl_handle_test.cc
View File

@@ -3984,14 +3984,11 @@ TEST_F(SpvParserHandleTest, TexelTypeWhenLoop) {
auto ast_body = fe.ast_body();
const auto got = test::ToString(p->program(), ast_body);
auto* expect = R"(var x_24 : vec2<f32>;
var x_24_phi_1 : vec2<f32>;
var x_26_phi_1 : i32;
x_24_phi_1 = vec2<f32>(0.0f, 0.0f);
x_26_phi_1 = 0i;
var x_26 : i32;
x_24 = vec2<f32>(0.0f, 0.0f);
x_26 = 0i;
loop {
var x_27 : i32;
x_24 = x_24_phi_1;
let x_26 : i32 = x_26_phi_1;
if ((x_26 < 2i)) {
} else {
break;
@@ -3999,8 +3996,8 @@ loop {
continuing {
x_27 = (x_26 + 1i);
x_24_phi_1 = vec2<f32>(1.0f, 1.0f);
x_26_phi_1 = x_27;
x_24 = vec2<f32>(1.0f, 1.0f);
x_26 = x_27;
}
}
textureStore(Output2Texture2D, vec2<i32>(vec2<u32>(1u, 1u)), vec4<f32>(x_24, 0.0f, 0.0f));
@@ -4060,14 +4057,11 @@ TEST_F(SpvParserHandleTest, SimpleSelectCanSelectFromHoistedConstant) {
auto ast_body = fe.ast_body();
const auto got = test::ToString(p->program(), ast_body);
auto* expect = R"(var x_14 : f32;
var x_14_phi_1 : f32;
var x_15_phi_1 : f32;
x_14_phi_1 = 0.0f;
x_15_phi_1 = 0.0f;
var x_15 : f32;
x_14 = 0.0f;
x_15 = 0.0f;
loop {
var x_17 : f32;
x_14 = x_14_phi_1;
let x_15 : f32 = x_15_phi_1;
if ((x_15 < 1.0f)) {
} else {
break;
@@ -4075,8 +4069,9 @@ loop {
continuing {
x_17 = (x_15 + 1.0f);
x_14_phi_1 = x_15;
x_15_phi_1 = x_17;
let x_15_c16_1 = x_15;
x_14 = x_15_c16_1;
x_15 = x_17;
}
}
let x_21 : f32 = select(0.0f, x_14, (x_14 > 1.0f));