Allow type-fluid GLSL-style vec2(int, bool) ctors in SkSL.
Note that GLSL accepts these sorts of constructors natively, but Metal
and SPIR-V do not. In the generated IR we actually add a cast for
subexpressions where the type does not match. These casts can be seen in
the final output for both GLSL (where they are no-ops) and Metal/SPIR-V
(where they are essential).
This change exposed some missing SPIR-V functionality (vector casts do
not support bool types). This can be fixed up in a followup CL; these
casts were previously disallowed by SkSL entirely, so there won't be any
of them in existing code.
Change-Id: I54ae922e91b38bed032537496428747a081dc774
Bug: skia:11164, skia:11171
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/353576
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index 62662c5..1e3e2bc 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -2126,7 +2126,7 @@
std::unique_ptr<Expression> IRGenerator::convertScalarConstructor(int offset,
const Type& type,
ExpressionArray args) {
- SkASSERT(type.isNumber() || type.isBoolean());
+ SkASSERT(type.isScalar());
if (args.size() != 1) {
this->errorReporter().error(
offset, "invalid arguments to '" + type.displayName() +
@@ -2135,31 +2135,20 @@
return nullptr;
}
- std::unique_ptr<Expression> converted = Constructor::SimplifyConversion(type, *args[0]);
- if (converted) {
- return converted;
- }
-
const Type& argType = args[0]->type();
- if (!argType.isNumber() && !argType.isBoolean()) {
+ if (!argType.isScalar()) {
this->errorReporter().error(
offset, "invalid argument to '" + type.displayName() +
"' constructor (expected a number or bool, but found '" +
argType.displayName() + "')");
return nullptr;
}
- return std::make_unique<Constructor>(offset, &type, std::move(args));
-}
-static int component_count(const Type& type) {
- switch (type.typeKind()) {
- case Type::TypeKind::kVector:
- return type.columns();
- case Type::TypeKind::kMatrix:
- return type.columns() * type.rows();
- default:
- return 1;
+ std::unique_ptr<Expression> converted = Constructor::SimplifyConversion(type, *args[0]);
+ if (converted) {
+ return converted;
}
+ return std::make_unique<Constructor>(offset, &type, std::move(args));
}
std::unique_ptr<Expression> IRGenerator::convertCompoundConstructor(int offset,
@@ -2167,49 +2156,62 @@
ExpressionArray args) {
SkASSERT(type.isVector() || type.isMatrix());
if (type.isMatrix() && args.size() == 1 && args[0]->type().isMatrix()) {
- // matrix from matrix is always legal
- return std::unique_ptr<Expression>(new Constructor(offset, &type, std::move(args)));
+ // Matrix-from-matrix is always legal.
+ return std::make_unique<Constructor>(offset, &type, std::move(args));
}
- int actual = 0;
+
+ if (args.size() == 1 && args[0]->type().isScalar()) {
+ // A constructor containing a single scalar is a splat (for vectors) or diagonal matrix (for
+ // matrices). In either event, it's legal regardless of the scalar's type. Synthesize an
+ // explicit conversion to the proper type (this is a no-op if it's unnecessary).
+ ExpressionArray castArgs;
+ castArgs.push_back(this->convertConstructor(offset, type.componentType(), std::move(args)));
+ return std::make_unique<Constructor>(offset, &type, std::move(castArgs));
+ }
+
int expected = type.rows() * type.columns();
- if (args.size() != 1 || expected != component_count(args[0]->type()) ||
- type.componentType().isNumber() != args[0]->type().componentType().isNumber()) {
- for (size_t i = 0; i < args.size(); i++) {
- const Type& argType = args[i]->type();
- if (argType.isVector()) {
- if (type.componentType().isNumber() !=
- argType.componentType().isNumber()) {
- this->errorReporter().error(offset,
- "'" + argType.displayName() +
+
+ if (type.isVector() && args.size() == 1 && args[0]->type().isVector() &&
+ args[0]->type().columns() == expected) {
+ // A vector constructor containing a single vector with the same number of columns is a
+ // cast (e.g. float3 -> int3).
+ return std::make_unique<Constructor>(offset, &type, std::move(args));
+ }
+
+ // For more complex cases, we walk the argument list and fix up the arguments as needed.
+ int actual = 0;
+ for (std::unique_ptr<Expression>& arg : args) {
+ if (!arg->type().isScalar() && !arg->type().isVector()) {
+ this->errorReporter().error(offset, "'" + arg->type().displayName() +
"' is not a valid parameter to '" +
type.displayName() + "' constructor");
- return nullptr;
- }
- actual += argType.columns();
- } else if (argType.isScalar()) {
- actual += 1;
- if (!type.isScalar()) {
- args[i] = this->coerce(std::move(args[i]), type.componentType());
- if (!args[i]) {
- return nullptr;
- }
- }
- } else {
- this->errorReporter().error(offset, "'" + argType.displayName() +
- "' is not a valid parameter to '" +
- type.displayName() + "' constructor");
- return nullptr;
- }
- }
- if (actual != 1 && actual != expected) {
- this->errorReporter().error(offset,
- "invalid arguments to '" + type.displayName() +
- "' constructor (expected " + to_string(expected) +
- " scalars, but found " + to_string(actual) + ")");
return nullptr;
}
+
+ // Rely on convertConstructor to force this subexpression to the proper type. If it's a
+ // literal, this will make sure it's the right type of literal. If an expression of
+ // matching type, the expression will be returned as-is. If it's an expression of
+ // mismatched type, this adds a cast.
+ int offset = arg->fOffset;
+ const Type& ctorType = type.componentType().toCompound(fContext, arg->type().columns(),
+ /*rows=*/1);
+ ExpressionArray ctorArg;
+ ctorArg.push_back(std::move(arg));
+ arg = this->convertConstructor(offset, ctorType, std::move(ctorArg));
+ if (!arg) {
+ return nullptr;
+ }
+ actual += ctorType.columns();
}
- return std::unique_ptr<Expression>(new Constructor(offset, &type, std::move(args)));
+
+ if (actual != expected) {
+ this->errorReporter().error(offset, "invalid arguments to '" + type.displayName() +
+ "' constructor (expected " + to_string(expected) +
+ " scalars, but found " + to_string(actual) + ")");
+ return nullptr;
+ }
+
+ return std::make_unique<Constructor>(offset, &type, std::move(args));
}
std::unique_ptr<Expression> IRGenerator::convertConstructor(int offset,
@@ -2220,7 +2222,7 @@
// Strip off redundant casts--i.e., convert Type(exprOfType) into exprOfType.
return std::move(args[0]);
}
- if (type.isNumber() || type.isBoolean()) {
+ if (type.isScalar()) {
return this->convertScalarConstructor(offset, type, std::move(args));
}
if (type.isVector() || type.isMatrix()) {
diff --git a/src/sksl/SkSLSPIRVCodeGenerator.cpp b/src/sksl/SkSLSPIRVCodeGenerator.cpp
index 52c99a5..fc2c759 100644
--- a/src/sksl/SkSLSPIRVCodeGenerator.cpp
+++ b/src/sksl/SkSLSPIRVCodeGenerator.cpp
@@ -1506,9 +1506,9 @@
if (argType.isVector()) {
// SPIR-V doesn't support vector(vector-of-different-type) directly, so we need to
// extract the components and convert them in that case manually. On top of that,
- // as of this writing there's a bug in the Intel Vulkan driver where OpCreateComposite
- // doesn't handle vector arguments at all, so we always extract vector components and
- // pass them into OpCreateComposite individually.
+ // as of this writing there's a bug in the Intel Vulkan driver where
+ // OpCompositeConstruct doesn't handle vector arguments at all, so we always extract
+ // vector components and pass them into OpCompositeConstruct individually.
SpvId vec = this->writeExpression(*c.arguments()[i], out);
SpvOp_ op = SpvOpUndef;
const Type& src = argType.componentType();
@@ -1527,7 +1527,9 @@
src == *fContext.fTypes.fUByte) {
op = SpvOpConvertUToF;
} else {
- SkASSERT(false);
+ fErrors.error(c.arguments()[i]->fOffset, "unsupported cast in SPIR-V: vector " +
+ src.description() + " to " +
+ dst.description());
}
} else if (dst == *fContext.fTypes.fInt ||
dst == *fContext.fTypes.fShort ||
@@ -1545,7 +1547,9 @@
src == *fContext.fTypes.fUByte) {
op = SpvOpBitcast;
} else {
- SkASSERT(false);
+ fErrors.error(c.arguments()[i]->fOffset, "unsupported cast in SPIR-V: vector " +
+ src.description() + " to " +
+ dst.description());
}
} else if (dst == *fContext.fTypes.fUInt ||
dst == *fContext.fTypes.fUShort ||
@@ -1563,7 +1567,9 @@
return vec;
}
} else {
- SkASSERT(false);
+ fErrors.error(c.arguments()[i]->fOffset, "unsupported cast in SPIR-V: vector " +
+ src.description() + " to " +
+ dst.description());
}
}
for (int j = 0; j < argType.columns(); j++) {
diff --git a/tests/sksl/fp/golden/GrMainCoords.cpp b/tests/sksl/fp/golden/GrMainCoords.cpp
index eafa859..3e1cac5 100644
--- a/tests/sksl/fp/golden/GrMainCoords.cpp
+++ b/tests/sksl/fp/golden/GrMainCoords.cpp
@@ -20,7 +20,7 @@
const GrMainCoords& _outer = args.fFp.cast<GrMainCoords>();
(void) _outer;
fragBuilder->codeAppendf(
-R"SkSL(return half4(%s, %s);
+R"SkSL(return half4(half2(%s), half2(%s));
)SkSL"
, args.fSampleCoord, args.fSampleCoord);
}
diff --git a/tests/sksl/shared/golden/VectorConstructors.asm.frag b/tests/sksl/shared/golden/VectorConstructors.asm.frag
index 0082e04..b4dc454 100644
--- a/tests/sksl/shared/golden/VectorConstructors.asm.frag
+++ b/tests/sksl/shared/golden/VectorConstructors.asm.frag
@@ -1,11 +1,4 @@
### Compilation failed:
-error: 10: expected 'int', but found 'float'
-error: 12: expected 'float', but found 'bool'
-error: 13: expected 'float', but found 'bool'
-error: 14: 'bool2' is not a valid parameter to 'float2' constructor
-error: 15: expected 'bool', but found 'float'
-error: 16: 'float2' is not a valid parameter to 'bool2' constructor
-error: 17: expected 'bool', but found 'float'
-error: 21: unknown identifier 'v10'
-8 errors
+error: 14: unsupported cast in SPIR-V: vector bool to float
+1 error
diff --git a/tests/sksl/shared/golden/VectorConstructors.glsl b/tests/sksl/shared/golden/VectorConstructors.glsl
index 0082e04..7737938 100644
--- a/tests/sksl/shared/golden/VectorConstructors.glsl
+++ b/tests/sksl/shared/golden/VectorConstructors.glsl
@@ -1,11 +1,22 @@
-### Compilation failed:
-error: 10: expected 'int', but found 'float'
-error: 12: expected 'float', but found 'bool'
-error: 13: expected 'float', but found 'bool'
-error: 14: 'bool2' is not a valid parameter to 'float2' constructor
-error: 15: expected 'bool', but found 'float'
-error: 16: 'float2' is not a valid parameter to 'bool2' constructor
-error: 17: expected 'bool', but found 'float'
-error: 21: unknown identifier 'v10'
-8 errors
+out vec4 sk_FragColor;
+vec2 v1 = vec2(1.0);
+vec2 v2 = vec2(1.0, 2.0);
+vec2 v3 = vec2(1.0);
+vec3 v4 = vec3(vec2(1.0), 1.0);
+ivec2 v5 = ivec2(1);
+ivec2 v6 = ivec2(vec2(1.0, 2.0));
+vec2 v7 = vec2(ivec2(1, 2));
+vec2 v8 = vec2(v5);
+vec4 v9 = vec4(float(v6.x), sqrt(2.0), vec2(ivec2(3, 4)));
+ivec2 v10 = ivec2(3, int(v1.x));
+bvec4 v11 = bvec4(bvec2(true, false), true, false);
+vec2 v12 = vec2(1.0, 0.0);
+vec2 v13 = vec2(0.0);
+vec2 v14 = vec2(bvec2(false));
+bvec2 v15 = bvec2(true);
+bvec2 v16 = bvec2(vec2(1.0));
+bvec3 v17 = bvec3(true, bvec2(ivec2(77)));
+void main() {
+ sk_FragColor.x = (((((((((((((((v1.x + v2.x) + v3.x) + v4.x) + float(v5.x)) + float(v6.x)) + v7.x) + v8.x) + v9.x) + float(v10.x)) + float(v11.x)) + v12.x) + v13.x) + v14.x) + float(v15.x)) + float(v16.x)) + float(v17.x);
+}
diff --git a/tests/sksl/shared/golden/VectorConstructors.metal b/tests/sksl/shared/golden/VectorConstructors.metal
index 0082e04..9626c59 100644
--- a/tests/sksl/shared/golden/VectorConstructors.metal
+++ b/tests/sksl/shared/golden/VectorConstructors.metal
@@ -1,11 +1,53 @@
-### Compilation failed:
+#include <metal_stdlib>
+#include <simd/simd.h>
+using namespace metal;
+struct Inputs {
+};
+struct Outputs {
+ float4 sk_FragColor [[color(0)]];
+};
+struct Globals {
+ float2 v1;
+ float2 v2;
+ float2 v3;
+ float3 v4;
+ int2 v5;
+ int2 v6;
+ float2 v7;
+ float2 v8;
+ float4 v9;
+ int2 v10;
+ bool4 v11;
+ float2 v12;
+ float2 v13;
+ float2 v14;
+ bool2 v15;
+ bool2 v16;
+ bool3 v17;
+};
-error: 10: expected 'int', but found 'float'
-error: 12: expected 'float', but found 'bool'
-error: 13: expected 'float', but found 'bool'
-error: 14: 'bool2' is not a valid parameter to 'float2' constructor
-error: 15: expected 'bool', but found 'float'
-error: 16: 'float2' is not a valid parameter to 'bool2' constructor
-error: 17: expected 'bool', but found 'float'
-error: 21: unknown identifier 'v10'
-8 errors
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+fragment Outputs fragmentMain(Inputs _in [[stage_in]], bool _frontFacing [[front_facing]], float4 _fragCoord [[position]]) {
+ Globals globalStruct{float2(1.0), float2(1.0, 2.0), float2(1.0), float3(float2(1.0), 1.0), int2(1), int2(float2(1.0, 2.0)), float2(int2(1, 2)), float2(_globals->v5), float4(float(_globals->v6.x), sqrt(2.0), float2(int2(3, 4))), int2(3, int(_globals->v1.x)), bool4(bool2(true, false), true, false), float2(1.0, 0.0), float2(0.0), float2(bool2(false)), bool2(true), bool2(float2(1.0)), bool3(true, bool2(int2(77)))};
+ thread Globals* _globals = &globalStruct;
+ (void)_globals;
+ Outputs _outputStruct;
+ thread Outputs* _out = &_outputStruct;
+ _out->sk_FragColor.x = (((((((((((((((_globals->v1.x + _globals->v2.x) + _globals->v3.x) + _globals->v4.x) + float(_globals->v5.x)) + float(_globals->v6.x)) + _globals->v7.x) + _globals->v8.x) + _globals->v9.x) + float(_globals->v10.x)) + float(_globals->v11.x)) + _globals->v12.x) + _globals->v13.x) + _globals->v14.x) + float(_globals->v15.x)) + float(_globals->v16.x)) + float(_globals->v17.x);
+ return *_out;
+}
