Align SkSL const rules more closely with GLSL
This adds Analysis::IsConstantExpression, to determine if an expression
is a constant-expression. It now expands to cover 'const' local and
global variables, because we also enforce that the initializer on those
variables is - in turn - a constant expression.
This fixes 10837 - previously you could initialize a const variable with
a non-constant expression, and we'd emit GLSL that contained that same
pattern, which would fail to compile at the driver level. That should
not be possible any longer.
Bug: skia:10679
Bug: skia:10837
Change-Id: I517820ef4da57fff45768c0b04c55aebc18d3272
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/375856
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
diff --git a/gn/sksl_tests.gni b/gn/sksl_tests.gni
index 084e829..88ba6f1 100644
--- a/gn/sksl_tests.gni
+++ b/gn/sksl_tests.gni
@@ -86,6 +86,7 @@
"/sksl/errors/ArrayUnspecifiedDimensions.sksl",
"/sksl/errors/AssignmentTypeMismatch.sksl",
"/sksl/errors/BadCaps.sksl",
+ "/sksl/errors/BadConstInitializers.sksl",
"/sksl/errors/BadFieldAccess.sksl",
"/sksl/errors/BadIndex.sksl",
"/sksl/errors/BadModifiers.sksl",
diff --git a/resources/sksl/errors/BadConstInitializers.sksl b/resources/sksl/errors/BadConstInitializers.sksl
new file mode 100644
index 0000000..5c45f76
--- /dev/null
+++ b/resources/sksl/errors/BadConstInitializers.sksl
@@ -0,0 +1,22 @@
+uniform float u;
+
+const float c = 1;
+ float f = 1;
+
+struct S { float f; } s;
+
+// Valid constant-expression initializers. Should not produce errors:
+void from_literal() { const float x = 0; }
+void from_const_global() { const float x = c; }
+void from_const_local() { const float x = 0; const float y = x; }
+void from_const_constructor() { const int i = int(c); }
+void from_expression() { const float x = 2 * c; }
+
+// Invalid constant-expression initializers. Should all produce errors:
+void from_uniform() { const float x = u; }
+void from_parameter(float p) { const float x = p; }
+void from_const_parameter(const float p) { const float x = p; }
+void from_non_const_local() { float x = u; const float y = x; }
+void from_non_const_expression() { const float x = u + u; }
+void from_mixed_expression() { const float x = c + u; }
+void from_non_const_struct_field() { const float x = s.f; }
diff --git a/resources/sksl/runtime_errors/IllegalIndexing.rte b/resources/sksl/runtime_errors/IllegalIndexing.rte
index 0a1ee83..9b3789a 100644
--- a/resources/sksl/runtime_errors/IllegalIndexing.rte
+++ b/resources/sksl/runtime_errors/IllegalIndexing.rte
@@ -1,7 +1,8 @@
-// Expect 4 errors
+// Expect 3 errors
// Legal cases that should not produce errors:
void literal_index() { int a[2]; a[0] = 0; a[1] = a[0]; }
+void const_var_index() { int a[2]; const int b = 0; a[b] = 0; }
void loop_index() { int a[2]; for (int i = 0; i < 2; ++i) { a[i] = i; } }
void loop_expr_binary() { int a[2]; for (int i = 0; i < 2; ++i) { a[1 - i] = i; } }
void loop_expr_unary() { int a[2]; for (int i = 0; i > -2; --i) { a[-i] = i; } }
@@ -26,4 +27,3 @@
// Legal in GLSL, not yet in SkSL:
void func_index() { int a[2]; a[int(abs(-1))] = 0; } // skbug.com/10835
-void const_var_index() { int a[2]; const int b = 0; a[b] = 0; } // skbug.com/10837
diff --git a/resources/sksl/shared/ConstVariableComparison.sksl b/resources/sksl/shared/ConstVariableComparison.sksl
index 177ebf3..80fdf95 100644
--- a/resources/sksl/shared/ConstVariableComparison.sksl
+++ b/resources/sksl/shared/ConstVariableComparison.sksl
@@ -3,7 +3,9 @@
half4 main() {
const float4 a = float4(0);
const float4 b = float4(1);
- const float4 c = abs(b); // still a Constant Expression, but SkSL cannot eliminate it (today)
+ // This is a constant-expression in GLSL, but not in SkSL (yet).
+ // We can't declare this const, and we can't eliminate it. skbug.com/10835
+ /*const*/ float4 c = abs(b);
if (a == b || b != c) {
return colorRed;
} else {
diff --git a/src/gpu/tessellate/GrStencilPathShader.cpp b/src/gpu/tessellate/GrStencilPathShader.cpp
index 0de7925..ba9039d 100644
--- a/src/gpu/tessellate/GrStencilPathShader.cpp
+++ b/src/gpu/tessellate/GrStencilPathShader.cpp
@@ -351,8 +351,8 @@
const auto& shader = args.fGP.cast<GrMiddleOutCubicShader>();
args.fVaryingHandler->emitAttributes(shader);
args.fVertBuilder->defineConstantf("int", "kMaxVertexID", "%i", 1 << kMaxResolveLevel);
- args.fVertBuilder->defineConstantf("float", "kInverseMaxVertexID", "exp2(-%i.0)",
- kMaxResolveLevel);
+ args.fVertBuilder->defineConstantf("float", "kInverseMaxVertexID",
+ "(1.0 / float(kMaxVertexID))");
args.fVertBuilder->insertFunction(kUnpackRationalCubicFn);
args.fVertBuilder->insertFunction(kEvalRationalCubicFn);
args.fVertBuilder->codeAppend(R"(
diff --git a/src/sksl/SkSLAnalysis.cpp b/src/sksl/SkSLAnalysis.cpp
index dbb95f6..8002ea5 100644
--- a/src/sksl/SkSLAnalysis.cpp
+++ b/src/sksl/SkSLAnalysis.cpp
@@ -732,9 +732,11 @@
return true;
}
-class ES2IndexExpressionVisitor : public ProgramVisitor {
+// Checks for ES2 constant-expression rules, and (optionally) constant-index-expression rules
+// (if loopIndices is non-nullptr)
+class ConstantExpressionVisitor : public ProgramVisitor {
public:
- ES2IndexExpressionVisitor(const std::set<const Variable*>* loopIndices)
+ ConstantExpressionVisitor(const std::set<const Variable*>* loopIndices)
: fLoopIndices(loopIndices) {}
bool visitExpression(const Expression& e) override {
@@ -746,14 +748,17 @@
case Expression::Kind::kFloatLiteral:
return false;
- // ... loop indices as defined in section 4. Today, SkSL allows no other variables,
- // but GLSL allows 'const' variables, because it requires that const variables be
- // initialized with constant-expressions. We could support that usage by checking
- // for variables that are const, and whose initializing expression also pass our
- // checks. For now, we'll be conservative. (skbug.com/10837)
- case Expression::Kind::kVariableReference:
- return fLoopIndices->find(e.as<VariableReference>().variable()) ==
- fLoopIndices->end();
+ // ... a global or local variable qualified as 'const', excluding function parameters.
+ // ... loop indices as defined in section 4. [constant-index-expression]
+ case Expression::Kind::kVariableReference: {
+ const Variable* v = e.as<VariableReference>().variable();
+ if ((v->storage() == Variable::Storage::kGlobal ||
+ v->storage() == Variable::Storage::kLocal) &&
+ (v->modifiers().fFlags & Modifiers::kConst_Flag)) {
+ return false;
+ }
+ return !fLoopIndices || fLoopIndices->find(v) == fLoopIndices->end();
+ }
// ... expressions composed of both of the above
case Expression::Kind::kBinary:
@@ -766,7 +771,7 @@
case Expression::Kind::kTernary:
return INHERITED::visitExpression(e);
- // These are completely disallowed in SkSL constant-index-expressions. GLSL allows
+ // These are completely disallowed in SkSL constant-(index)-expressions. GLSL allows
// calls to built-in functions where the arguments are all constant-expressions, but
// we don't guarantee that behavior. (skbug.com/10835)
case Expression::Kind::kExternalFunctionCall:
@@ -811,7 +816,7 @@
bool visitExpression(const Expression& e) override {
if (e.is<IndexExpression>()) {
const IndexExpression& i = e.as<IndexExpression>();
- ES2IndexExpressionVisitor indexerInvalid(&fLoopIndices);
+ ConstantExpressionVisitor indexerInvalid(&fLoopIndices);
if (indexerInvalid.visitExpression(*i.index())) {
fErrors.error(i.fOffset, "index expression must be constant");
return true;
@@ -834,6 +839,11 @@
visitor.visitProgramElement(pe);
}
+bool Analysis::IsConstantExpression(const Expression& expr) {
+ ConstantExpressionVisitor visitor(/*loopIndices=*/nullptr);
+ return !visitor.visitExpression(expr);
+}
+
////////////////////////////////////////////////////////////////////////////////
// ProgramVisitor
diff --git a/src/sksl/SkSLAnalysis.h b/src/sksl/SkSLAnalysis.h
index ee78274..190593b 100644
--- a/src/sksl/SkSLAnalysis.h
+++ b/src/sksl/SkSLAnalysis.h
@@ -83,6 +83,21 @@
// - myStruct.myArrayField[7].xyz
static bool IsTrivialExpression(const Expression& expr);
+ // Is 'expr' a constant-expression, as defined by GLSL 1.0, section 5.10? A constant expression
+ // is one of:
+ //
+ // - A literal value
+ // - A global or local variable qualified as 'const', excluding function parameters
+ // - An expression formed by an operator on operands that are constant expressions, including
+ // getting an element of a constant vector or a constant matrix, or a field of a constant
+ // structure
+ // - A constructor whose arguments are all constant expressions
+ //
+ // GLSL (but not SkSL, yet - skbug.com/10835) also provides:
+ // - A built-in function call whose arguments are all constant expressions, with the exception
+ // of the texture lookup functions
+ static bool IsConstantExpression(const Expression& expr);
+
struct UnrollableLoopInfo {
const Variable* fIndex;
double fStart;
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index d1f954e..c6881fd 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -406,9 +406,16 @@
std::unique_ptr<Statement> IRGenerator::convertVarDeclaration(std::unique_ptr<Variable> var,
std::unique_ptr<Expression> value) {
- if ((var->modifiers().fFlags & Modifiers::kConst_Flag) && !value) {
- this->errorReporter().error(var->fOffset, "'const' variables must be initialized");
- return nullptr;
+ if (var->modifiers().fFlags & Modifiers::kConst_Flag) {
+ if (!value) {
+ this->errorReporter().error(var->fOffset, "'const' variables must be initialized");
+ return nullptr;
+ }
+ if (!Analysis::IsConstantExpression(*value)) {
+ this->errorReporter().error(
+ value->fOffset, "'const' variable initializer must be a constant expression");
+ return nullptr;
+ }
}
if (value) {
if (var->type().isOpaque()) {
diff --git a/tests/sksl/errors/BadConstInitializers.glsl b/tests/sksl/errors/BadConstInitializers.glsl
new file mode 100644
index 0000000..25fcb94
--- /dev/null
+++ b/tests/sksl/errors/BadConstInitializers.glsl
@@ -0,0 +1,10 @@
+### Compilation failed:
+
+error: 16: 'const' variable initializer must be a constant expression
+error: 17: 'const' variable initializer must be a constant expression
+error: 18: 'const' variable initializer must be a constant expression
+error: 19: 'const' variable initializer must be a constant expression
+error: 20: 'const' variable initializer must be a constant expression
+error: 21: 'const' variable initializer must be a constant expression
+error: 22: 'const' variable initializer must be a constant expression
+7 errors
diff --git a/tests/sksl/runtime_errors/IllegalIndexing.skvm b/tests/sksl/runtime_errors/IllegalIndexing.skvm
index a04fd78..cf4620b 100644
--- a/tests/sksl/runtime_errors/IllegalIndexing.skvm
+++ b/tests/sksl/runtime_errors/IllegalIndexing.skvm
@@ -1,7 +1,6 @@
### Compilation failed:
-error: 24: index expression must be constant
error: 25: index expression must be constant
-error: 28: index expression must be constant
+error: 26: index expression must be constant
error: 29: index expression must be constant
-4 errors
+3 errors
diff --git a/tests/sksl/shared/ConstVariableComparison.glsl b/tests/sksl/shared/ConstVariableComparison.glsl
index 5022fba..176f9f0 100644
--- a/tests/sksl/shared/ConstVariableComparison.glsl
+++ b/tests/sksl/shared/ConstVariableComparison.glsl
@@ -3,7 +3,7 @@
uniform vec4 colorGreen;
uniform vec4 colorRed;
vec4 main() {
- const vec4 c = abs(vec4(1.0));
+ vec4 c = abs(vec4(1.0));
if (vec4(1.0) != c) {
return colorRed;
} else {
diff --git a/tests/sksl/shared/ConstVariableComparison.metal b/tests/sksl/shared/ConstVariableComparison.metal
index 577347d..5473abb 100644
--- a/tests/sksl/shared/ConstVariableComparison.metal
+++ b/tests/sksl/shared/ConstVariableComparison.metal
@@ -15,7 +15,7 @@
fragment Outputs fragmentMain(Inputs _in [[stage_in]], constant Uniforms& _uniforms [[buffer(0)]], bool _frontFacing [[front_facing]], float4 _fragCoord [[position]]) {
Outputs _out;
(void)_out;
- const float4 c = abs(float4(1.0));
+ float4 c = abs(float4(1.0));
if (any(float4(1.0) != c)) {
_out.sk_FragColor = _uniforms.colorRed;
return _out;
