Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SPIR-V: prune unreachable merge block and continue target #1943

Merged
merged 1 commit into from
Nov 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions SPIRV/GlslangToSpv.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1630,11 +1630,11 @@ void TGlslangToSpvTraverser::finishSpv()
for (auto it = iOSet.cbegin(); it != iOSet.cend(); ++it)
entryPoint->addIdOperand(*it);

#ifndef GLSLANG_WEB
// Add capabilities, extensions, remove unneeded decorations, etc.,
// Add capabilities, extensions, remove unneeded decorations, etc.,
// based on the resulting SPIR-V.
// Note: WebGPU code generation must have the opportunity to aggressively
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This turned on lots of code having to do with types and capabilities that should not be relevant to WebGPU.

Don't we need to just turn on the CFG topology part?

I can look at that too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, just reply with the specific subset needed by WebGPU, and I'll do the experiment/pruning of other things and verify how size is impacted.

(First, I'll merge this, and then do that as a second step to get space back for WebGPU.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to just turn on the CFG topology part?

In principle, yes.

But when I looked more deeply at Builder::postProcess, I became worried that the WEB path was missing functionality, particularly around adding per-instruction capabilities and extensions.
Looking again, at the moment those additional things are only affecting:

  • 8bit and 16bit storage
  • physical storage
    Neither of these are in the WEB variant for now, so that could be saved.
    I can take on getting that code space back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I posted #1968 to reclaim the codespace wasted by this part of the change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks like it solves it, thanks.

// prune unreachable merge blocks and continue targets.
builder.postProcess();
#endif
}

// Write the SPV into 'out'.
Expand Down
44 changes: 31 additions & 13 deletions SPIRV/InReadableOrder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,22 @@ namespace {
// Use by calling visit() on the root block.
class ReadableOrderTraverser {
public:
explicit ReadableOrderTraverser(std::function<void(Block*)> callback) : callback_(callback) {}
ReadableOrderTraverser(std::function<void(Block*, spv::ReachReason, Block*)> callback)
: callback_(callback) {}
// Visits the block if it hasn't been visited already and isn't currently
// being delayed. Invokes callback(block), then descends into its
// being delayed. Invokes callback(block, why, header), then descends into its
// successors. Delays merge-block and continue-block processing until all
// the branches have been completed.
void visit(Block* block)
// the branches have been completed. If |block| is an unreachable merge block or
// an unreachable continue target, then |header| is the corresponding header block.
void visit(Block* block, spv::ReachReason why, Block* header)
{
assert(block);
if (why == spv::ReachViaControlFlow) {
reachableViaControlFlow_.insert(block);
}
if (visited_.count(block) || delayed_.count(block))
return;
callback_(block);
callback_(block, why, header);
visited_.insert(block);
Block* mergeBlock = nullptr;
Block* continueBlock = nullptr;
Expand All @@ -87,27 +92,40 @@ class ReadableOrderTraverser {
delayed_.insert(continueBlock);
}
}
const auto successors = block->getSuccessors();
for (auto it = successors.cbegin(); it != successors.cend(); ++it)
visit(*it);
if (why == spv::ReachViaControlFlow) {
const auto& successors = block->getSuccessors();
for (auto it = successors.cbegin(); it != successors.cend(); ++it)
visit(*it, why, nullptr);
}
if (continueBlock) {
const spv::ReachReason continueWhy =
(reachableViaControlFlow_.count(continueBlock) > 0)
? spv::ReachViaControlFlow
: spv::ReachDeadContinue;
delayed_.erase(continueBlock);
visit(continueBlock);
visit(continueBlock, continueWhy, block);
}
if (mergeBlock) {
const spv::ReachReason mergeWhy =
(reachableViaControlFlow_.count(mergeBlock) > 0)
? spv::ReachViaControlFlow
: spv::ReachDeadMerge;
delayed_.erase(mergeBlock);
visit(mergeBlock);
visit(mergeBlock, mergeWhy, block);
}
}

private:
std::function<void(Block*)> callback_;
std::function<void(Block*, spv::ReachReason, Block*)> callback_;
// Whether a block has already been visited or is being delayed.
std::unordered_set<Block *> visited_, delayed_;

// The set of blocks that actually are reached via control flow.
std::unordered_set<Block *> reachableViaControlFlow_;
};
}

void spv::inReadableOrder(Block* root, std::function<void(Block*)> callback)
void spv::inReadableOrder(Block* root, std::function<void(Block*, spv::ReachReason, Block*)> callback)
{
ReadableOrderTraverser(callback).visit(root);
ReadableOrderTraverser(callback).visit(root, spv::ReachViaControlFlow, nullptr);
}
4 changes: 1 addition & 3 deletions SPIRV/SpvBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -683,14 +683,12 @@ class Builder {
// based on the type of the base and the chain of dereferences.
Id accessChainGetInferredType();

// Add capabilities, extensions, remove unneeded decorations, etc.,
// Add capabilities, extensions, remove unneeded decorations, etc.,
// based on the resulting SPIR-V.
void postProcess();

// Hook to visit each instruction in a block in a function
void postProcess(Instruction&);
// Hook to visit each instruction in a reachable block in a function.
void postProcessReachable(const Instruction&);
// Hook to visit each non-32-bit sized float/int operation in a block.
void postProcessType(const Instruction&, spv::Id typeId);

Expand Down
49 changes: 33 additions & 16 deletions SPIRV/SpvPostProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include <cassert>
#include <cstdlib>

#include <unordered_map>
#include <unordered_set>
#include <algorithm>

Expand Down Expand Up @@ -319,33 +320,56 @@ void Builder::postProcess(Instruction& inst)
}
}

// Called for each instruction in a reachable block.
void Builder::postProcessReachable(const Instruction&)
{
// did have code here, but questionable to do so without deleting the instructions
}

// comment in header
void Builder::postProcess()
{
// reachableBlocks is the set of blockss reached via control flow, or which are
// unreachable continue targert or unreachable merge.
std::unordered_set<const Block*> reachableBlocks;
std::unordered_map<Block*, Block*> headerForUnreachableContinue;
std::unordered_set<Block*> unreachableMerges;
std::unordered_set<Id> unreachableDefinitions;
// Collect IDs defined in unreachable blocks. For each function, label the
// reachable blocks first. Then for each unreachable block, collect the
// result IDs of the instructions in it.
for (auto fi = module.getFunctions().cbegin(); fi != module.getFunctions().cend(); fi++) {
Function* f = *fi;
Block* entry = f->getEntryBlock();
inReadableOrder(entry, [&reachableBlocks](const Block* b) { reachableBlocks.insert(b); });
inReadableOrder(entry,
[&reachableBlocks, &unreachableMerges, &headerForUnreachableContinue]
(Block* b, ReachReason why, Block* header) {
reachableBlocks.insert(b);
if (why == ReachDeadContinue) headerForUnreachableContinue[b] = header;
if (why == ReachDeadMerge) unreachableMerges.insert(b);
});
for (auto bi = f->getBlocks().cbegin(); bi != f->getBlocks().cend(); bi++) {
Block* b = *bi;
if (reachableBlocks.count(b) == 0) {
for (auto ii = b->getInstructions().cbegin(); ii != b->getInstructions().cend(); ii++)
if (unreachableMerges.count(b) != 0 || headerForUnreachableContinue.count(b) != 0) {
auto ii = b->getInstructions().cbegin();
++ii; // Keep potential decorations on the label.
for (; ii != b->getInstructions().cend(); ++ii)
unreachableDefinitions.insert(ii->get()->getResultId());
} else if (reachableBlocks.count(b) == 0) {
// The normal case for unreachable code. All definitions are considered dead.
for (auto ii = b->getInstructions().cbegin(); ii != b->getInstructions().cend(); ++ii)
johnkslang marked this conversation as resolved.
Show resolved Hide resolved
unreachableDefinitions.insert(ii->get()->getResultId());
}
}
}

// Modify unreachable merge blocks and unreachable continue targets.
// Delete their contents.
for (auto mergeIter = unreachableMerges.begin(); mergeIter != unreachableMerges.end(); ++mergeIter) {
(*mergeIter)->rewriteAsCanonicalUnreachableMerge();
}
for (auto continueIter = headerForUnreachableContinue.begin();
continueIter != headerForUnreachableContinue.end();
++continueIter) {
Block* continue_target = continueIter->first;
Block* header = continueIter->second;
continue_target->rewriteAsCanonicalUnreachableContinue(header);
}

// Remove unneeded decorations, for unreachable instructions
decorations.erase(std::remove_if(decorations.begin(), decorations.end(),
[&unreachableDefinitions](std::unique_ptr<Instruction>& I) -> bool {
Expand Down Expand Up @@ -374,13 +398,6 @@ void Builder::postProcess()
}
}

// process all reachable instructions...
for (auto bi = reachableBlocks.cbegin(); bi != reachableBlocks.cend(); ++bi) {
const Block* block = *bi;
const auto function = [this](const std::unique_ptr<Instruction>& inst) { postProcessReachable(*inst.get()); };
std::for_each(block->getInstructions().begin(), block->getInstructions().end(), function);
}

// process all block-contained instructions
for (auto fi = module.getFunctions().cbegin(); fi != module.getFunctions().cend(); fi++) {
Function* f = *fi;
Expand Down
51 changes: 48 additions & 3 deletions SPIRV/spvIR.h
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,36 @@ class Block {
return nullptr;
}

// Change this block into a canonical dead merge block. Delete instructions
// as necessary. A canonical dead merge block has only an OpLabel and an
// OpUnreachable.
void rewriteAsCanonicalUnreachableMerge() {
assert(localVariables.empty());
// Delete all instructions except for the label.
assert(instructions.size() > 0);
instructions.resize(1);
successors.clear();
Instruction* unreachable = new Instruction(OpUnreachable);
addInstruction(std::unique_ptr<Instruction>(unreachable));
}
// Change this block into a canonical dead continue target branching to the
// given header ID. Delete instructions as necessary. A canonical dead continue
// target has only an OpLabel and an unconditional branch back to the corresponding
// header.
void rewriteAsCanonicalUnreachableContinue(Block* header) {
assert(localVariables.empty());
// Delete all instructions except for the label.
assert(instructions.size() > 0);
instructions.resize(1);
successors.clear();
// Add OpBranch back to the header.
assert(header != nullptr);
Instruction* branch = new Instruction(OpBranch);
branch->addIdOperand(header->getId());
addInstruction(std::move(std::unique_ptr<Instruction>(branch)));
successors.push_back(header);
}

bool isTerminated() const
{
switch (instructions.back()->getOpCode()) {
Expand All @@ -235,6 +265,7 @@ class Block {
case OpKill:
case OpReturn:
case OpReturnValue:
case OpUnreachable:
return true;
default:
return false;
Expand Down Expand Up @@ -268,10 +299,24 @@ class Block {
bool unreachable;
};

// The different reasons for reaching a block in the inReadableOrder traversal.
typedef enum ReachReason {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of typedef here without also declaring a name? I'm getting warnings on this have no purpose. Does it work just as well to remove the typedef?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, I removed the typedef.

// Reachable from the entry block via transfers of control, i.e. branches.
ReachViaControlFlow = 0,
// A continue target that is not reachable via control flow.
ReachDeadContinue,
// A merge block that is not reachable via control flow.
ReachDeadMerge
};

// Traverses the control-flow graph rooted at root in an order suited for
// readable code generation. Invokes callback at every node in the traversal
// order.
void inReadableOrder(Block* root, std::function<void(Block*)> callback);
// order. The callback arguments are:
// - the block,
// - the reason we reached the block,
// - if the reason was that block is an unreachable continue or unreachable merge block
// then the last parameter is the corresponding header block.
void inReadableOrder(Block* root, std::function<void(Block*, ReachReason, Block* header)> callback);

//
// SPIR-V IR Function.
Expand Down Expand Up @@ -321,7 +366,7 @@ class Function {
parameterInstructions[p]->dump(out);

// Blocks
inReadableOrder(blocks[0], [&out](const Block* b) { b->dump(out); });
inReadableOrder(blocks[0], [&out](const Block* b, ReachReason, Block*) { b->dump(out); });
Instruction end(0, 0, OpFunctionEnd);
end.dump(out);
}
Expand Down
2 changes: 1 addition & 1 deletion StandAlone/StandAlone.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -988,7 +988,7 @@ void CompileAndLinkShaderUnits(std::vector<ShaderCompUnit> compUnits)

// Set base bindings
shader->setShiftBinding(res, baseBinding[res][compUnit.stage]);

// Set bindings for particular resource sets
// TODO: use a range based for loop here, when available in all environments.
for (auto i = baseBindingForSet[res][compUnit.stage].begin();
Expand Down
3 changes: 1 addition & 2 deletions Test/baseResults/hlsl.constantbuffer.frag.out
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,5 @@ Validation failed
60: 7(fvec4) CompositeConstruct 59 59 59 59
ReturnValue 60
30: Label
62: 7(fvec4) Undef
ReturnValue 62
Unreachable
FunctionEnd
Loading