From a6cc82e3cf01780e9e1cc4e0d824daf07ec40ae5 Mon Sep 17 00:00:00 2001 From: John Stiles Date: Thu, 29 Apr 2021 17:41:00 +0000 Subject: [PATCH] Revert "Optimize away swizzles of constant variables." This reverts commit 7b253d34ee0ef9fc514b030dab76aac940b70325. Reason for revert: asserting on some bots Original change's description: > Optimize away swizzles of constant variables. > > Change-Id: I49807f18ea54e85c2b8f1419278c54aa2d6f8fac > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/402581 > Commit-Queue: John Stiles > Auto-Submit: John Stiles > Reviewed-by: Ethan Nicholas TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com Change-Id: I530e6923e3c56f503508c70258c25f160f8985bc No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://skia-review.googlesource.com/c/skia/+/402640 Reviewed-by: John Stiles Commit-Queue: John Stiles --- src/sksl/ir/SkSLSwizzle.cpp | 29 ++++++++++++++---------- tests/sksl/errors/InvalidAssignment.glsl | 2 +- tests/sksl/folding/SwizzleFolding.glsl | 11 ++++++++- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/sksl/ir/SkSLSwizzle.cpp b/src/sksl/ir/SkSLSwizzle.cpp index fec8d2e64a..25263610b9 100644 --- a/src/sksl/ir/SkSLSwizzle.cpp +++ b/src/sksl/ir/SkSLSwizzle.cpp @@ -5,7 +5,6 @@ * found in the LICENSE file. */ -#include "src/sksl/SkSLConstantFolder.h" #include "src/sksl/ir/SkSLConstructor.h" #include "src/sksl/ir/SkSLConstructorScalarCast.h" #include "src/sksl/ir/SkSLConstructorSplat.h" @@ -184,24 +183,20 @@ std::unique_ptr Swizzle::Make(const Context& context, return Swizzle::Make(context, std::move(base.base()), combined); } - // If we are swizzling a constant expression, we can use its value instead here (so that - // swizzles like `colorWhite.x` can be simplified to `1`). - const Expression* value = ConstantFolder::GetConstantValueForVariable(*expr); - // `half4(scalar).zyy` can be optimized to `half3(scalar)`, and `half3(scalar).y` can be // optimized to just `scalar`. The swizzle components don't actually matter, as every field // in a splat constructor holds the same value. - if (value->is()) { - const ConstructorSplat& splat = value->as(); + if (expr->is()) { + ConstructorSplat& splat = expr->as(); return ConstructorSplat::Make( context, splat.fOffset, splat.type().componentType().toCompound(context, components.size(), /*rows=*/1), - splat.argument()->clone()); + std::move(splat.argument())); } // Optimize swizzles of constructors. - if (value->isAnyConstructor()) { - const AnyConstructor& base = value->asAnyConstructor(); + if (expr->isAnyConstructor()) { + AnyConstructor& base = expr->asAnyConstructor(); auto baseArguments = base.argumentSpan(); std::unique_ptr replacement; const Type& componentType = exprType.componentType(); @@ -304,8 +299,18 @@ std::unique_ptr Swizzle::Make(const Context& context, ExpressionArray newArgs; newArgs.reserve_back(swizzleSize); for (const ReorderedArgument& reorderedArg : reorderedArgs) { - std::unique_ptr newArg = - baseArguments[reorderedArg.fArgIndex]->clone(); + std::unique_ptr& origArg = baseArguments[reorderedArg.fArgIndex]; + + // Clone the original argument if there are multiple references to it; just + // steal it if there's only one reference left. + std::unique_ptr newArg; + int8_t& exprRemainingRefs = exprUsed[reorderedArg.fArgIndex]; + SkASSERT(exprRemainingRefs > 0); + if (--exprRemainingRefs == 0) { + newArg = std::move(origArg); + } else { + newArg = origArg->clone(); + } if (reorderedArg.fComponents.empty()) { newArgs.push_back(std::move(newArg)); diff --git a/tests/sksl/errors/InvalidAssignment.glsl b/tests/sksl/errors/InvalidAssignment.glsl index e81d4d9c91..92a11e2473 100644 --- a/tests/sksl/errors/InvalidAssignment.glsl +++ b/tests/sksl/errors/InvalidAssignment.glsl @@ -3,7 +3,7 @@ error: 7: cannot assign to this expression error: 8: cannot modify immutable variable 'u' error: 9: cannot modify immutable variable 'x' -error: 11: cannot assign to this expression +error: 11: cannot modify immutable variable 'x' error: 12: cannot write to the same swizzle field more than once error: 14: cannot modify immutable variable 'l' error: 15: cannot modify immutable variable 'r' diff --git a/tests/sksl/folding/SwizzleFolding.glsl b/tests/sksl/folding/SwizzleFolding.glsl index 1d080b0592..af7c9f32c6 100644 --- a/tests/sksl/folding/SwizzleFolding.glsl +++ b/tests/sksl/folding/SwizzleFolding.glsl @@ -3,7 +3,16 @@ out vec4 sk_FragColor; uniform vec4 colorRed; uniform vec4 colorGreen; vec4 main() { + const vec4 _0_colorWhite = vec4(1.0); + const vec2 _1_point = vec2(40.0, 60.0); bool _2_ok = true; - _2_ok = _2_ok && colorGreen != colorRed; + _2_ok = _2_ok && (((_1_point.x >= 0.0 && _1_point.x <= 100.0) && _1_point.y >= 0.0) && _1_point.y <= 100.0); + _2_ok = _2_ok && _0_colorWhite.x == 1.0; + _2_ok = _2_ok && _0_colorWhite.x + _0_colorWhite.y == 2.0; + _2_ok = _2_ok && (_0_colorWhite.x + _0_colorWhite.y) + _0_colorWhite.z == 3.0; + _2_ok = _2_ok && ((_0_colorWhite.x + _0_colorWhite.y) + _0_colorWhite.z) + _0_colorWhite.w == 4.0; + _2_ok = _2_ok && colorGreen * _0_colorWhite.x != colorRed * _0_colorWhite.y; + const vec2 _3_pointOffset = _1_point.yx + _0_colorWhite.xz; + _2_ok = _2_ok && _3_pointOffset == vec2(61.0, 41.0); return _2_ok ? colorGreen : colorRed; }