Demonstrate a bug with inlined static switches.
When we detect a static switch, the optimizer finds the matching switch-
case and eliminates all the other switch-cases. It handles case
fall-through by scanning forward and looking for an unconditional break.
However, the inliner has an interesting quirk--it can replace `return`
statements inside of a switch with `continue` statements, since the body
of the inlined function has been wrapped with a for-loop to allow for
early exits. The optimizer does not recognize these continue statements
as exits from the switch (although they certainly qualify), so it
treats continues as fallen-through and keeps emitting switch-cases.
The dead-code elimination pass was actually doing us a favor here and
eliminating the excess code later. A flag was added to disable DCE in
order to reveal the problem in a test.
Change-Id: I8ff19fde5e32d0ab73d7c5411da40cb953a446f5
Bug: skia:11352
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/372956
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni
index 301161b..4306f3c 100644
--- a/gn/sksl_tests.gni
+++ b/gn/sksl_tests.gni
@@ -479,6 +479,7 @@
"/sksl/inliner/InlinerWrapsEarlyReturnsWithForLoop.sksl",
"/sksl/inliner/InlinerWrapsSwitchWithReturnInsideWithForLoop.sksl",
"/sksl/inliner/ShortCircuitEvaluationsCannotInlineRightHandSide.sksl",
+ "/sksl/inliner/StaticSwitch.sksl",
"/sksl/inliner/StructsCanBeInlinedSafely.sksl",
"/sksl/inliner/SwitchWithCastCanBeInlined.sksl",
"/sksl/inliner/SwitchWithoutReturnInsideCanBeInlined.sksl",
diff --git a/resources/sksl/inliner/StaticSwitch.sksl b/resources/sksl/inliner/StaticSwitch.sksl
new file mode 100644
index 0000000..1ef785a
--- /dev/null
+++ b/resources/sksl/inliner/StaticSwitch.sksl
@@ -0,0 +1,19 @@
+/*#pragma settings NoDeadCodeElimination*/
+
+uniform half4 colorGreen, colorRed;
+
+float get() {
+ @switch (2) {
+ case 1: return abs(1);
+ case 2: return abs(2); // Only this case should be preserved.
+ case 3: return abs(3);
+ case 4: return abs(4);
+ }
+ // This won't be removed because dead-code elimination is disabled.
+ return abs(5);
+}
+
+half4 main() {
+ float result = get();
+ return result == 2 ? colorGreen : colorRed;
+}
diff --git a/src/sksl/SkSLCompiler.cpp b/src/sksl/SkSLCompiler.cpp
index fb3da30..1b2a469 100644
--- a/src/sksl/SkSLCompiler.cpp
+++ b/src/sksl/SkSLCompiler.cpp
@@ -732,7 +732,8 @@
case Expression::Kind::kVariableReference: {
const VariableReference& ref = expr->as<VariableReference>();
const Variable* var = ref.variable();
- if (ref.refKind() != VariableReference::RefKind::kWrite &&
+ if (fContext->fConfig->fSettings.fDeadCodeElimination &&
+ ref.refKind() != VariableReference::RefKind::kWrite &&
ref.refKind() != VariableReference::RefKind::kPointer &&
var->storage() == Variable::Storage::kLocal && !definitions.get(var) &&
optimizationContext->fSilences.find(var) == optimizationContext->fSilences.end()) {
@@ -1481,14 +1482,16 @@
CFG cfg = CFGGenerator().getCFG(f);
this->computeDataFlow(&cfg);
- // check for unreachable code
- for (size_t i = 0; i < cfg.fBlocks.size(); i++) {
- const BasicBlock& block = cfg.fBlocks[i];
- if (!block.fIsReachable && !block.fAllowUnreachable && block.fNodes.size()) {
- const BasicBlock::Node& node = block.fNodes[0];
- int offset = node.isStatement() ? (*node.statement())->fOffset
- : (*node.expression())->fOffset;
- this->error(offset, String("unreachable"));
+ if (fContext->fConfig->fSettings.fDeadCodeElimination) {
+ // Check for unreachable code.
+ for (size_t i = 0; i < cfg.fBlocks.size(); i++) {
+ const BasicBlock& block = cfg.fBlocks[i];
+ if (!block.fIsReachable && !block.fAllowUnreachable && block.fNodes.size()) {
+ const BasicBlock::Node& node = block.fNodes[0];
+ int offset = node.isStatement() ? (*node.statement())->fOffset
+ : (*node.expression())->fOffset;
+ this->error(offset, String("unreachable"));
+ }
}
}
if (fErrorCount) {
@@ -1518,24 +1521,27 @@
}
BasicBlock& b = cfg.fBlocks[blockId];
- if (blockId > 0 && !b.fIsReachable) {
- // Block was reachable before optimization, but has since become unreachable. In
- // addition to being dead code, it's broken - since control flow can't reach it, no
- // prior variable definitions can reach it, and therefore variables might look to
- // have not been properly assigned. Kill it by replacing all statements with Nops.
- for (BasicBlock::Node& node : b.fNodes) {
- if (node.isStatement() && !(*node.statement())->is<Nop>()) {
- // Eliminating a node runs the risk of eliminating that node's exits as
- // well. Keep track of this and do a rescan if we are about to access one
- // of these.
- for (BlockId id : b.fExits) {
- eliminatedBlockIds.set(id);
+ if (fContext->fConfig->fSettings.fDeadCodeElimination) {
+ if (blockId > 0 && !b.fIsReachable) {
+ // Block was reachable before optimization, but has since become unreachable. In
+ // addition to being dead code, it's broken - since control flow can't reach it,
+ // no prior variable definitions can reach it, and therefore variables might
+ // look to have not been properly assigned. Kill it by replacing all statements
+ // with Nops.
+ for (BasicBlock::Node& node : b.fNodes) {
+ if (node.isStatement() && !(*node.statement())->is<Nop>()) {
+ // Eliminating a node runs the risk of eliminating that node's exits as
+ // well. Keep track of this and do a rescan if we are about to access
+ // one of these.
+ for (BlockId id : b.fExits) {
+ eliminatedBlockIds.set(id);
+ }
+ node.setStatement(std::make_unique<Nop>(), usage);
+ madeChanges = true;
}
- node.setStatement(std::make_unique<Nop>(), usage);
- madeChanges = true;
}
+ continue;
}
- continue;
}
DefinitionMap definitions = b.fBefore;
diff --git a/src/sksl/SkSLMain.cpp b/src/sksl/SkSLMain.cpp
index 6af3ce1..b178145 100644
--- a/src/sksl/SkSLMain.cpp
+++ b/src/sksl/SkSLMain.cpp
@@ -204,6 +204,9 @@
static auto s_version450CoreCaps = Factory::Version450Core();
*caps = s_version450CoreCaps.get();
}
+ if (settingsText.consumeSuffix(" NoDeadCodeElimination")) {
+ settings->fDeadCodeElimination = false;
+ }
if (settingsText.consumeSuffix(" FlipY")) {
settings->fFlipY = true;
}
diff --git a/src/sksl/SkSLProgramSettings.h b/src/sksl/SkSLProgramSettings.h
index bc13775..219624a 100644
--- a/src/sksl/SkSLProgramSettings.h
+++ b/src/sksl/SkSLProgramSettings.h
@@ -51,14 +51,17 @@
// At present, zero is always used by our backends.
int fDefaultUniformSet = 0;
int fDefaultUniformBinding = 0;
- // If true, remove any uncalled functions other than main(). Note that a function which
- // starts out being used may end up being uncalled after optimization.
- bool fRemoveDeadFunctions = true;
- // Sets an upper limit on the acceptable amount of code growth from inlining.
- // A value of zero will disable the inliner entirely.
- int fInlineThreshold = SkSL::kDefaultInlineThreshold;
- // true to enable optimization passes
+ // Enables the SkSL optimizer.
bool fOptimize = true;
+ // (Requires fOptimize = true) Remove any uncalled functions other than main(). Note that a
+ // function which starts out being used may end up being uncalled after optimization.
+ bool fRemoveDeadFunctions = true;
+ // (Requires fOptimize = true) Uses the control-flow graph to detect and eliminate code within
+ // a function that has become unreachable due to optimization.
+ bool fDeadCodeElimination = true;
+ // (Requires fOptimize = true) When greater than zero, enables the inliner. The threshold value
+ // sets an upper limit on the acceptable amount of code growth from inlining.
+ int fInlineThreshold = SkSL::kDefaultInlineThreshold;
// If true, implicit conversions to lower precision numeric types are allowed
// (eg, float to half)
bool fAllowNarrowingConversions = false;
diff --git a/src/sksl/SkSLSPIRVCodeGenerator.cpp b/src/sksl/SkSLSPIRVCodeGenerator.cpp
index 7610011..2b061eb 100644
--- a/src/sksl/SkSLSPIRVCodeGenerator.cpp
+++ b/src/sksl/SkSLSPIRVCodeGenerator.cpp
@@ -198,7 +198,7 @@
case SpvOpSwitch: // fall through
case SpvOpBranch: // fall through
case SpvOpBranchConditional:
- SkASSERT(fCurrentBlock);
+ SkASSERT(fCurrentBlock || !fContext.fConfig->fSettings.fDeadCodeElimination);
fCurrentBlock = 0;
break;
case SpvOpConstant: // fall through
@@ -236,7 +236,7 @@
case SpvOpMemberDecorate:
break;
default:
- SkASSERT(fCurrentBlock);
+ SkASSERT(fCurrentBlock || !fContext.fConfig->fSettings.fDeadCodeElimination);
}
this->writeWord((length << 16) | opCode, out);
}
diff --git a/tests/sksl/inliner/StaticSwitch.glsl b/tests/sksl/inliner/StaticSwitch.glsl
new file mode 100644
index 0000000..2775fc0
--- /dev/null
+++ b/tests/sksl/inliner/StaticSwitch.glsl
@@ -0,0 +1,30 @@
+
+out vec4 sk_FragColor;
+uniform vec4 colorGreen;
+uniform vec4 colorRed;
+vec4 main() {
+ float _0_get;
+ for (int _1_loop = 0;_1_loop < 1; _1_loop++) {
+ {
+ {
+ _0_get = abs(2.0);
+ continue;
+ }
+ {
+ _0_get = abs(3.0);
+ continue;
+ }
+ {
+ _0_get = abs(4.0);
+ continue;
+ }
+ }
+ {
+ _0_get = abs(5.0);
+ continue;
+ }
+ }
+ float result = _0_get;
+
+ return result == 2.0 ? colorGreen : colorRed;
+}
