Disallow inlining ternary true/false branches.

This prevents unnecessary work from being done, and also prevents us
from executing side-effects from the wrong side.

Change-Id: I4dbf3974388807f15e9eadb2abf1b1243d047ce2
Bug: skia:10688
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/314797
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>

Author: johnstiles-google

src/sksl/SkSLIRGenerator.cpp

@@ -1879,13 +1879,18 @@ std::unique_ptr<Expression> IRGenerator::convertTernaryExpression(const ASTNode&
     if (!test) {
         return nullptr;
     }
-    std::unique_ptr<Expression> ifTrue = this->convertExpression(*(iter++));
-    if (!ifTrue) {
-        return nullptr;
-    }
-    std::unique_ptr<Expression> ifFalse = this->convertExpression(*(iter++));
-    if (!ifFalse) {
-        return nullptr;
+    std::unique_ptr<Expression> ifTrue;
+    std::unique_ptr<Expression> ifFalse;
+    {
+        AutoDisableInline disableInline(this);
+        ifTrue = this->convertExpression(*(iter++));
+        if (!ifTrue) {
+            return nullptr;
+        }
+        ifFalse = this->convertExpression(*(iter++));
+        if (!ifFalse) {
+            return nullptr;
+        }
     }
     const Type* trueType;
     const Type* falseType;
@@ -1918,10 +1923,10 @@ std::unique_ptr<Expression> IRGenerator::convertTernaryExpression(const ASTNode&
             return ifFalse;
         }
     }
-    return std::unique_ptr<Expression>(new TernaryExpression(node.fOffset,
-                                                             std::move(test),
-                                                             std::move(ifTrue),
-                                                             std::move(ifFalse)));
+    return std::make_unique<TernaryExpression>(node.fOffset,
+                                               std::move(test),
+                                               std::move(ifTrue),
+                                               std::move(ifFalse));
 }
 
 void IRGenerator::copyIntrinsicIfNeeded(const FunctionDeclaration& function) {

tests/SkSLFPTest.cpp

@@ -1088,9 +1088,6 @@ R"SkSL(%s = %s(%s);
 }
 
 DEF_TEST(SkSLFPTernaryExpressionsShouldNotInlineResults, r) {
-    // NOTE: this test exposes a bug with the inliner. We don't want to inline both sides of a
-    // ternary, since only one side needs to be evaluated, and side effects from the wrong side
-    // must not occur.
     test(r,
          *SkSL::ShaderCapsFactory::Default(),
          R"__SkSL__(
@@ -1114,30 +1111,30 @@ DEF_TEST(SkSLFPTernaryExpressionsShouldNotInlineResults, r) {
          )__SkSL__",
          /*expectedH=*/{},
          /*expectedCPP=*/{
-         R"__Cpp__(fragBuilder->codeAppendf(
+         R"__Cpp__(SkString trueSide_name;
+        const GrShaderVar trueSide_args[] = { GrShaderVar("v", kHalf4_GrSLType)};
+        fragBuilder->emitFunction(kHalf4_GrSLType, "trueSide", 1, trueSide_args,
+R"SkSL(count += 1.0;
+return half4(sin(v.x), sin(v.y), sin(v.z), sin(v.w));
+)SkSL", &trueSide_name);
+        SkString falseSide_name;
+        const GrShaderVar falseSide_args[] = { GrShaderVar("v", kHalf4_GrSLType)};
+        fragBuilder->emitFunction(kHalf4_GrSLType, "falseSide", 1, falseSide_args,
+R"SkSL(count += 1.0;
+return half4(cos(v.y), cos(v.z), cos(v.w), cos(v.z));
+)SkSL", &falseSide_name);
+        fragBuilder->codeAppendf(
 R"SkSL(half count = %f;
 bool _0_test;
 {
     _0_test = %s.x <= 0.5;
 }
 
-half4 _1_trueSide;
-{
-    count += 1.0;
-    _1_trueSide = half4(sin(%s.x), sin(%s.y), sin(%s.z), sin(%s.w));
-}
-
-half4 _2_falseSide;
-{
-    count += 1.0;
-    _2_falseSide = half4(cos(%s.y), cos(%s.z), cos(%s.w), cos(%s.z));
-}
-
-%s = _0_test ? _1_trueSide : _2_falseSide;
+%s = _0_test ? %s(%s) : %s(%s);
 
 %s *= count;
 )SkSL"
-, count, args.fUniformHandler->getUniformCStr(colorVar), args.fUniformHandler->getUniformCStr(colorVar), args.fUniformHandler->getUniformCStr(colorVar), args.fUniformHandler->getUniformCStr(colorVar), args.fUniformHandler->getUniformCStr(colorVar), args.fUniformHandler->getUniformCStr(colorVar), args.fUniformHandler->getUniformCStr(colorVar), args.fUniformHandler->getUniformCStr(colorVar), args.fUniformHandler->getUniformCStr(colorVar), args.fOutputColor, args.fOutputColor);
+, count, args.fUniformHandler->getUniformCStr(colorVar), args.fOutputColor, trueSide_name.c_str(), args.fUniformHandler->getUniformCStr(colorVar), falseSide_name.c_str(), args.fUniformHandler->getUniformCStr(colorVar), args.fOutputColor);
 )__Cpp__"});
 }