From 83973655240a73b88ae69210e606220144c61744 Mon Sep 17 00:00:00 2001 From: John Stiles Date: Thu, 7 Apr 2022 14:36:09 -0400 Subject: [PATCH] Optimize away same-value ternaries. A ternary of the form `anything ? value : value` can be reduced to a comma-expression of the form `anything, value`. This seems like a rare case in real code, but it's easy enough to detect with our existing toolbox. The `anything` test-expression will be eliminated from the expression if it has no side effects, using our existing constant-folding rules for the comma expression. Change-Id: I1285b04cd6a08f1bed614aa1aa6f37ea2447de91 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528439 Auto-Submit: John Stiles Reviewed-by: Brian Osman Commit-Queue: John Stiles --- gn/sksl_tests.gni | 1 + resources/sksl/folding/TernaryFolding.sksl | 29 ++++++++++++++++++++++ src/sksl/ir/BUILD.bazel | 2 ++ src/sksl/ir/SkSLTernaryExpression.cpp | 14 +++++++++++ tests/SkSLTest.cpp | 1 + tests/sksl/folding/TernaryFolding.glsl | 17 +++++++++++++ 6 files changed, 64 insertions(+) create mode 100644 resources/sksl/folding/TernaryFolding.sksl create mode 100644 tests/sksl/folding/TernaryFolding.glsl diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni index 8c2b68e432..fd848862f4 100644 --- a/gn/sksl_tests.gni +++ b/gn/sksl_tests.gni @@ -538,6 +538,7 @@ sksl_folding_tests = [ "/sksl/folding/ShortCircuitBoolFolding.sksl", "/sksl/folding/SwizzleFolding.sksl", "/sksl/folding/SwitchCaseFolding.sksl", + "/sksl/folding/TernaryFolding.sksl", "/sksl/folding/VectorScalarFolding.sksl", "/sksl/folding/VectorVectorFolding.sksl", ] diff --git a/resources/sksl/folding/TernaryFolding.sksl b/resources/sksl/folding/TernaryFolding.sksl new file mode 100644 index 0000000000..22ef9a276f --- /dev/null +++ b/resources/sksl/folding/TernaryFolding.sksl @@ -0,0 +1,29 @@ +uniform half4 colorRed, colorGreen; + +bool do_side_effect(out bool x) { + x = true; + return false; +} + +const bool TRUE = true; +const bool FALSE = false; + +half4 main(float2 coords) { + bool ok; + + ok = (colorRed == colorGreen) ? true : true; + + ok = ok && (colorGreen.g == 1 ? true : true); + ok = ok && (colorGreen.g == 0 ? TRUE : true); + ok = ok || (colorGreen.g == 1 ? false : false); + ok = ok || (colorGreen.g == 0 ? false : FALSE); + + half4 green = coords.x == coords.y ? colorGreen : colorGreen; + half4 red = coords.x != coords.y ? colorRed : colorRed; + + // Make sure side effects are honored. + bool param = false; + bool call = do_side_effect(param) ? true : true; + + return (ok && param && call) ? green : red; +} diff --git a/src/sksl/ir/BUILD.bazel b/src/sksl/ir/BUILD.bazel index 50b6f02763..93c1001c57 100644 --- a/src/sksl/ir/BUILD.bazel +++ b/src/sksl/ir/BUILD.bazel @@ -999,10 +999,12 @@ generated_cc_atom( srcs = ["SkSLTernaryExpression.cpp"], visibility = ["//:__subpackages__"], deps = [ + ":SkSLBinaryExpression_hdr", ":SkSLLiteral_hdr", ":SkSLTernaryExpression_hdr", "//include/sksl:SkSLErrorReporter_hdr", "//include/sksl:SkSLOperator_hdr", + "//src/sksl:SkSLAnalysis_hdr", "//src/sksl:SkSLConstantFolder_hdr", "//src/sksl:SkSLContext_hdr", "//src/sksl:SkSLProgramSettings_hdr", diff --git a/src/sksl/ir/SkSLTernaryExpression.cpp b/src/sksl/ir/SkSLTernaryExpression.cpp index c6ef772737..c8368b5a85 100644 --- a/src/sksl/ir/SkSLTernaryExpression.cpp +++ b/src/sksl/ir/SkSLTernaryExpression.cpp @@ -9,9 +9,11 @@ #include "include/sksl/SkSLErrorReporter.h" #include "include/sksl/SkSLOperator.h" +#include "src/sksl/SkSLAnalysis.h" #include "src/sksl/SkSLConstantFolder.h" #include "src/sksl/SkSLContext.h" #include "src/sksl/SkSLProgramSettings.h" +#include "src/sksl/ir/SkSLBinaryExpression.h" #include "src/sksl/ir/SkSLLiteral.h" namespace SkSL { @@ -80,6 +82,18 @@ std::unique_ptr TernaryExpression::Make(const Context& context, } } + // A ternary with matching true- and false-cases can be reduced to `(test, ifTrue)`. + // If `test` has no side-effects, it will be optimized away by the constant-folder as well. + if (context.fConfig->fSettings.fOptimize) { + const Expression* ifTrueExpr = ConstantFolder::GetConstantValueForVariable(*ifTrue); + const Expression* ifFalseExpr = ConstantFolder::GetConstantValueForVariable(*ifFalse); + + if (Analysis::IsSameExpressionTree(*ifTrueExpr, *ifFalseExpr)) { + return BinaryExpression::Make(context, pos, std::move(test), + Operator::Kind::COMMA, std::move(ifTrue)); + } + } + return std::make_unique(pos, std::move(test), std::move(ifTrue), std::move(ifFalse)); } diff --git a/tests/SkSLTest.cpp b/tests/SkSLTest.cpp index c3304912e6..826cd8942b 100644 --- a/tests/SkSLTest.cpp +++ b/tests/SkSLTest.cpp @@ -352,6 +352,7 @@ SKSL_TEST(CPU + GPU + SkQP, SelfAssignment, "folding/SelfAssign SKSL_TEST(CPU + GPU + SkQP, ShortCircuitBoolFolding, "folding/ShortCircuitBoolFolding.sksl") SKSL_TEST(CPU + GPU + SkQP, SwitchCaseFolding, "folding/SwitchCaseFolding.sksl") SKSL_TEST(CPU + GPU + SkQP, SwizzleFolding, "folding/SwizzleFolding.sksl") +SKSL_TEST(CPU + GPU + SkQP, TernaryFolding, "folding/TernaryFolding.sksl") SKSL_TEST(CPU + GPU + SkQP, VectorScalarFolding, "folding/VectorScalarFolding.sksl") SKSL_TEST(CPU + GPU + SkQP, VectorVectorFolding, "folding/VectorVectorFolding.sksl") diff --git a/tests/sksl/folding/TernaryFolding.glsl b/tests/sksl/folding/TernaryFolding.glsl new file mode 100644 index 0000000000..e82b23b8a6 --- /dev/null +++ b/tests/sksl/folding/TernaryFolding.glsl @@ -0,0 +1,17 @@ + +out vec4 sk_FragColor; +uniform vec4 colorRed; +uniform vec4 colorGreen; +bool do_side_effect_bb(out bool x) { + x = true; + return false; +} +vec4 main() { + bool ok; + ok = true; + vec4 green = colorGreen; + vec4 red = colorRed; + bool param = false; + bool call = (do_side_effect_bb(param), true); + return (ok && param) && call ? green : red; +}