Migrate setRefKind assignability checker into SkSLAnalysis.
This will allow the inliner to use IsAssignable.
Change-Id: Ic94f71002779b53d0b3dc97f37fbe4bb98b026d8
Bug: skia:10756
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319414
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni
index 4114803..61d39ba 100644
--- a/gn/sksl_tests.gni
+++ b/gn/sksl_tests.gni
@@ -110,7 +110,6 @@
"$_tests/sksl/errors/SwitchDuplicateCase.sksl",
"$_tests/sksl/errors/SwitchTypes.sksl",
"$_tests/sksl/errors/SwizzleConstantOutput.sksl",
- "$_tests/sksl/errors/SwizzleDuplicateOutput.sksl",
"$_tests/sksl/errors/SwizzleMatrix.sksl",
"$_tests/sksl/errors/SwizzleOnlyLiterals.sksl",
"$_tests/sksl/errors/SwizzleOutOfBounds.sksl",
diff --git a/src/sksl/SkSLAnalysis.cpp b/src/sksl/SkSLAnalysis.cpp
index 87647c7..ef6ed59 100644
--- a/src/sksl/SkSLAnalysis.cpp
+++ b/src/sksl/SkSLAnalysis.cpp
@@ -8,6 +8,7 @@
#include "src/sksl/SkSLAnalysis.h"
#include "include/private/SkSLSampleUsage.h"
+#include "src/sksl/SkSLErrorReporter.h"
#include "src/sksl/ir/SkSLExpression.h"
#include "src/sksl/ir/SkSLProgram.h"
#include "src/sksl/ir/SkSLProgramElement.h"
@@ -206,6 +207,91 @@
using INHERITED = ProgramVisitor;
};
+// This isn't actually using ProgramVisitor, because it only considers a subset of the fields for
+// any given expression kind. For instance, when indexing an array (e.g. `x[1]`), we only want to
+// know if the base (`x`) is assignable; the index expression (`1`) doesn't need to be.
+class IsAssignableVisitor {
+public:
+ IsAssignableVisitor(std::vector<VariableReference*>* assignableVars, ErrorReporter& errors)
+ : fAssignableVars(assignableVars)
+ , fErrors(errors) {}
+
+ bool visit(Expression& expr) {
+ this->visitExpression(expr);
+ return fErrors.errorCount() == 0;
+ }
+
+ void visitExpression(Expression& expr) {
+ switch (expr.kind()) {
+ case Expression::Kind::kVariableReference: {
+ VariableReference& varRef = expr.as<VariableReference>();
+ const Variable* var = varRef.fVariable;
+ if (var->fModifiers.fFlags & (Modifiers::kConst_Flag | Modifiers::kUniform_Flag |
+ Modifiers::kVarying_Flag)) {
+ fErrors.error(expr.fOffset,
+ "cannot modify immutable variable '" + var->fName + "'");
+ } else if (fAssignableVars) {
+ fAssignableVars->push_back(&varRef);
+ }
+ break;
+ }
+ case Expression::Kind::kFieldAccess:
+ this->visitExpression(*expr.as<FieldAccess>().fBase);
+ break;
+
+ case Expression::Kind::kSwizzle: {
+ const Swizzle& swizzle = expr.as<Swizzle>();
+ this->checkSwizzleWrite(swizzle);
+ this->visitExpression(*swizzle.fBase);
+ break;
+ }
+ case Expression::Kind::kIndex:
+ this->visitExpression(*expr.as<IndexExpression>().fBase);
+ break;
+
+ case Expression::Kind::kTernary: {
+ const TernaryExpression& ternary = expr.as<TernaryExpression>();
+ this->visitExpression(*ternary.fIfTrue);
+ this->visitExpression(*ternary.fIfFalse);
+ break;
+ }
+ case Expression::Kind::kExternalValue: {
+ const ExternalValue* var = expr.as<ExternalValueReference>().fValue;
+ if (!var->canWrite()) {
+ fErrors.error(expr.fOffset,
+ "cannot modify immutable external value '" + var->fName + "'");
+ }
+ break;
+ }
+ default:
+ fErrors.error(expr.fOffset, "cannot assign to this expression");
+ break;
+ }
+ }
+
+private:
+ void checkSwizzleWrite(const Swizzle& swizzle) {
+ int bits = 0;
+ for (int idx : swizzle.fComponents) {
+ SkASSERT(idx <= 3);
+ int bit = 1 << idx;
+ if (bits & bit) {
+ fErrors.error(swizzle.fOffset,
+ "cannot write to the same swizzle field more than once");
+ break;
+ }
+ bits |= bit;
+ }
+ }
+
+ std::vector<VariableReference*>* fAssignableVars;
+ ErrorReporter& fErrors;
+
+ using INHERITED = ProgramVisitor;
+};
+
+
+
} // namespace
////////////////////////////////////////////////////////////////////////////////
@@ -237,6 +323,11 @@
return VariableWriteVisitor(&var).visit(stmt);
}
+bool Analysis::IsAssignable(Expression& expr, std::vector<VariableReference*>* assignableVars,
+ ErrorReporter& errors) {
+ return IsAssignableVisitor{assignableVars, errors}.visit(expr);
+}
+
////////////////////////////////////////////////////////////////////////////////
// ProgramVisitor
diff --git a/src/sksl/SkSLAnalysis.h b/src/sksl/SkSLAnalysis.h
index eedb337..1a627c2 100644
--- a/src/sksl/SkSLAnalysis.h
+++ b/src/sksl/SkSLAnalysis.h
@@ -8,17 +8,21 @@
#ifndef SkSLAnalysis_DEFINED
#define SkSLAnalysis_DEFINED
+#include <vector>
+
#include "include/private/SkSLSampleUsage.h"
#include "src/sksl/SkSLDefines.h"
namespace SkSL {
+class ErrorReporter;
struct Expression;
struct FunctionDefinition;
struct Program;
struct ProgramElement;
struct Statement;
struct Variable;
+struct VariableReference;
/**
* Provides utilities for analyzing SkSL statically before it's composed into a full program.
@@ -34,6 +38,8 @@
static int NodeCount(const FunctionDefinition& function);
static bool StatementWritesToVariable(const Statement& stmt, const Variable& var);
+ static bool IsAssignable(Expression& expr, std::vector<VariableReference*>* assignableVars,
+ ErrorReporter& errors);
};
/**
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index a7a0882..f92c6b3 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -2810,60 +2810,17 @@
}
}
-bool IRGenerator::checkSwizzleWrite(const Swizzle& swizzle) {
- int bits = 0;
- for (int idx : swizzle.fComponents) {
- SkASSERT(idx <= 3);
- int bit = 1 << idx;
- if (bits & bit) {
- fErrors.error(swizzle.fOffset,
- "cannot write to the same swizzle field more than once");
- return false;
- }
- bits |= bit;
+bool IRGenerator::setRefKind(Expression& expr, VariableReference::RefKind kind) {
+ std::vector<VariableReference*> assignableVars;
+ if (!Analysis::IsAssignable(expr, &assignableVars, fErrors)) {
+ return false;
+ }
+ for (VariableReference* v : assignableVars) {
+ v->setRefKind(kind);
}
return true;
}
-bool IRGenerator::setRefKind(Expression& expr, VariableReference::RefKind kind) {
- switch (expr.kind()) {
- case Expression::Kind::kVariableReference: {
- const Variable& var = *expr.as<VariableReference>().fVariable;
- if (var.fModifiers.fFlags &
- (Modifiers::kConst_Flag | Modifiers::kUniform_Flag | Modifiers::kVarying_Flag)) {
- fErrors.error(expr.fOffset, "cannot modify immutable variable '" + var.fName + "'");
- return false;
- }
- expr.as<VariableReference>().setRefKind(kind);
- return true;
- }
- case Expression::Kind::kFieldAccess:
- return this->setRefKind(*expr.as<FieldAccess>().fBase, kind);
- case Expression::Kind::kSwizzle: {
- const Swizzle& swizzle = expr.as<Swizzle>();
- return this->checkSwizzleWrite(swizzle) && this->setRefKind(*swizzle.fBase, kind);
- }
- case Expression::Kind::kIndex:
- return this->setRefKind(*expr.as<IndexExpression>().fBase, kind);
- case Expression::Kind::kTernary: {
- const TernaryExpression& t = expr.as<TernaryExpression>();
- return this->setRefKind(*t.fIfTrue, kind) && this->setRefKind(*t.fIfFalse, kind);
- }
- case Expression::Kind::kExternalValue: {
- const ExternalValue& v = *expr.as<ExternalValueReference>().fValue;
- if (!v.canWrite()) {
- fErrors.error(expr.fOffset,
- "cannot modify immutable external value '" + v.fName + "'");
- return false;
- }
- return true;
- }
- default:
- fErrors.error(expr.fOffset, "cannot assign to this expression");
- return false;
- }
-}
-
void IRGenerator::convertProgram(Program::Kind kind,
const char* text,
size_t length,
diff --git a/src/sksl/SkSLIRGenerator.h b/src/sksl/SkSLIRGenerator.h
index 1a0a928..a19947f 100644
--- a/src/sksl/SkSLIRGenerator.h
+++ b/src/sksl/SkSLIRGenerator.h
@@ -198,7 +198,6 @@
void checkValid(const Expression& expr);
bool setRefKind(Expression& expr, VariableReference::RefKind kind);
bool getConstantInt(const Expression& value, int64_t* out);
- bool checkSwizzleWrite(const Swizzle& swizzle);
void copyIntrinsicIfNeeded(const FunctionDeclaration& function);
Inliner* fInliner = nullptr;
diff --git a/tests/sksl/errors/InvalidAssignment.sksl b/tests/sksl/errors/InvalidAssignment.sksl
index 064988b..8e95fa7 100644
--- a/tests/sksl/errors/InvalidAssignment.sksl
+++ b/tests/sksl/errors/InvalidAssignment.sksl
@@ -1,6 +1,19 @@
-uniform int a;
-const int b;
+struct S {
+ float f;
+};
-void assign_to_literal() { 1 = 2; }
-void assign_to_uniform() { a = 0; }
-void assign_to_const() { b = 0; }
+uniform int u;
+
+void assign_to_literal() { 1 = 2; }
+void assign_to_uniform() { u = 0; }
+void assign_to_const() { const int x; x = 0; }
+void assign_to_const_array() { const int x[1]; x[0] = 0; }
+void assign_to_const_swizzle() { const half4 x; x.w = 0; }
+void assign_to_repeated_swizzle() { half4 x; x.yy = half2(0); }
+void assign_to_const_struct() { const S s; s.f = 0; }
+void assign_to_ternary() { float l; float r; (true ? l : r) = 0; } // TODO(skbug.com/10767)
+void assign_to_ternary_const_left() { const float l; float r; (true ? l : r) = 0; }
+void assign_to_ternary_const_right() { float l; const float r; (false ? l : r) = 0; }
+void assign_to_ternary_const_both() { const float l; const float r; (true ? l : r) = 0; }
+void assign_to_unary_minus() { float x; -x = 0; }
+void assign_to_unary_plus() { float x; +x = 0; } // TODO(skbug.com/10766)
diff --git a/tests/sksl/errors/SwizzleDuplicateOutput.sksl b/tests/sksl/errors/SwizzleDuplicateOutput.sksl
deleted file mode 100644
index 37e2e0a..0000000
--- a/tests/sksl/errors/SwizzleDuplicateOutput.sksl
+++ /dev/null
@@ -1,4 +0,0 @@
-void main() {
- float4 test = float4(1);
- test.xyyz = float4(1);
-}
diff --git a/tests/sksl/errors/golden/InvalidAssignment.glsl b/tests/sksl/errors/golden/InvalidAssignment.glsl
index f7fcc0e..1bb4da4 100644
--- a/tests/sksl/errors/golden/InvalidAssignment.glsl
+++ b/tests/sksl/errors/golden/InvalidAssignment.glsl
@@ -1,6 +1,14 @@
### Compilation failed:
-error: 4: cannot assign to this expression
-error: 5: cannot modify immutable variable 'a'
-error: 6: cannot modify immutable variable 'b'
-3 errors
+error: 7: cannot assign to this expression
+error: 8: cannot modify immutable variable 'u'
+error: 9: cannot modify immutable variable 'x'
+error: 10: cannot modify immutable variable 'x'
+error: 11: cannot modify immutable variable 'x'
+error: 12: cannot write to the same swizzle field more than once
+error: 13: cannot modify immutable variable 's'
+error: 15: cannot modify immutable variable 'l'
+error: 16: cannot modify immutable variable 'r'
+error: 17: cannot modify immutable variable 'l'
+error: 18: cannot assign to this expression
+11 errors
diff --git a/tests/sksl/errors/golden/SwizzleDuplicateOutput.glsl b/tests/sksl/errors/golden/SwizzleDuplicateOutput.glsl
deleted file mode 100644
index 03bed6e..0000000
--- a/tests/sksl/errors/golden/SwizzleDuplicateOutput.glsl
+++ /dev/null
@@ -1,4 +0,0 @@
-### Compilation failed:
-
-error: 3: cannot write to the same swizzle field more than once
-1 error
