Allow precision qualifiers in function params.

Previously, `Type::applyPrecisionQualifiers` would return a new type
(e.g. `mediump + float` returned `half`) but left the precision
qualifier flags as-is. This was implemented that way because the
modifiers were already baked into a pool, so mutating them was
difficult.

The rewritten DSLParser does not share this limitation--every place
where applyPrecisionQualifiers is used, the Modifiers are easily
mutable. As a result, `applyPrecisionQualifiers` can now clear the
precision-qualifier bits on the Modifier, meaning that `half` and a
`mediump float` will generate the exact same Type/Modifier combination.

This change fixes a bug where precision qualifiers were not allowed on
function parameters. (See `check_parameters` in FunctionDeclaration.cpp
to pinpoint the cause of the error. A less-invasive fix could have just
marked those modifier bits as allowed in `check_parameters`, but this
fix addresses the root of the issue and is honestly how I wanted
`applyPrecisionQualifiers` to work all along.)

Change-Id: I331813efa54138f469a0d5bff2d274cd3ce64b70
Bug: skia:12489
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/455156
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>

include/sksl/DSLType.h

@@ -88,7 +88,8 @@ public:
 
     DSLType(skstd::string_view name);
 
-    DSLType(skstd::string_view name, const DSLModifiers& modifiers,
+    DSLType(skstd::string_view name,
+            DSLModifiers* modifiers,
             PositionInfo pos = PositionInfo::Capture());
 
     /**

resources/sksl/runtime/PrecisionQualifiers.rts

@@ -48,6 +48,10 @@ bool test_array() {
     return mf[0] == hf[0] && hv[0] == mv[0] && mv[1] == hv[1];
 }
 
+bool highp_param  (highp   float value) { return value == 1; }
+bool mediump_param(mediump float value) { return value == 2; }
+bool lowp_param   (lowp    float value) { return value == 3; }
+
 vec4 main(vec2 coords) {
     highp   vec4 zero = vec4(0);
     mediump vec4 one = vec4(1);
@@ -57,5 +61,6 @@ vec4 main(vec2 coords) {
     highp vec4 red = colorRed;
     red = (red + zero) * one;
 
-    return (test_scalar() && test_vector() && test_matrix() && test_array()) ? green : red;
+    return (test_scalar() && test_vector() && test_matrix() && test_array() &&
+            highp_param(1) && mediump_param(2) && lowp_param(3)) ? green : red;
 }

src/sksl/SkSLDSLParser.cpp

@@ -321,7 +321,7 @@ bool DSLParser::declaration() {
         this->structVarDeclaration(modifiers);
         return true;
     }
-    skstd::optional<DSLType> type = this->type(modifiers);
+    skstd::optional<DSLType> type = this->type(&modifiers);
     if (!type) {
         return false;
     }
@@ -551,7 +551,7 @@ DSLStatement DSLParser::varDeclarationsOrExpressionStatement() {
 bool DSLParser::varDeclarationsPrefix(VarDeclarationsPrefix* prefixData) {
     prefixData->fPosition = this->position(this->peek());
     prefixData->fModifiers = this->modifiers();
-    skstd::optional<DSLType> type = this->type(prefixData->fModifiers);
+    skstd::optional<DSLType> type = this->type(&prefixData->fModifiers);
     if (!type) {
         return false;
     }
@@ -588,8 +588,7 @@ skstd::optional<DSLType> DSLParser::structDeclaration() {
     SkTArray<DSLField> fields;
     while (!this->checkNext(Token::Kind::TK_RBRACE)) {
         DSLModifiers modifiers = this->modifiers();
-
+        skstd::optional<DSLType> type = this->type(&modifiers);
-        skstd::optional<DSLType> type = this->type(modifiers);
         if (!type) {
             return skstd::nullopt;
         }
@@ -639,7 +638,7 @@ SkTArray<dsl::DSLGlobalVar> DSLParser::structVarDeclaration(const DSLModifiers&
 /* modifiers type IDENTIFIER (LBRACKET INT_LITERAL RBRACKET)? */
 skstd::optional<DSLWrapper<DSLParameter>> DSLParser::parameter() {
     DSLModifiers modifiers = this->modifiersWithDefaults(0);
-    skstd::optional<DSLType> type = this->type(modifiers);
+    skstd::optional<DSLType> type = this->type(&modifiers);
     if (!type) {
         return skstd::nullopt;
     }
@@ -833,7 +832,7 @@ DSLStatement DSLParser::statement() {
 }
 
 /* IDENTIFIER(type) (LBRACKET intLiteral? RBRACKET)* QUESTION? */
-skstd::optional<DSLType> DSLParser::type(const DSLModifiers& modifiers) {
+skstd::optional<DSLType> DSLParser::type(DSLModifiers* modifiers) {
     Token type;
     if (!this->expect(Token::Kind::TK_IDENTIFIER, "a type", &type)) {
         return skstd::nullopt;
@@ -873,7 +872,7 @@ bool DSLParser::interfaceBlock(const dsl::DSLModifiers& modifiers) {
     SkTArray<dsl::Field> fields;
     while (!this->checkNext(Token::Kind::TK_RBRACE)) {
         DSLModifiers fieldModifiers = this->modifiers();
-        skstd::optional<dsl::DSLType> type = this->type(fieldModifiers);
+        skstd::optional<dsl::DSLType> type = this->type(&fieldModifiers);
         if (!type) {
             return false;
         }

src/sksl/SkSLDSLParser.h

@@ -188,7 +188,7 @@ private:
 
     dsl::DSLStatement statement();
 
-    skstd::optional<dsl::DSLType> type(const dsl::DSLModifiers& modifiers);
+    skstd::optional<dsl::DSLType> type(dsl::DSLModifiers* modifiers);
 
     bool interfaceBlock(const dsl::DSLModifiers& mods);
 

src/sksl/dsl/DSLType.cpp

@@ -41,7 +41,7 @@ static const Type* find_type(skstd::string_view name, PositionInfo pos) {
     return &result;
 }
 
-static const Type* find_type(skstd::string_view name, const Modifiers& modifiers,
+static const Type* find_type(skstd::string_view name, Modifiers* modifiers,
         PositionInfo pos) {
     const Type* type = find_type(name, pos);
     if (!type) {
@@ -56,8 +56,8 @@ static const Type* find_type(skstd::string_view name, const Modifiers& modifiers
 DSLType::DSLType(skstd::string_view name)
         : fSkSLType(find_type(name, PositionInfo())) {}
 
-DSLType::DSLType(skstd::string_view name, const DSLModifiers& modifiers, PositionInfo position)
+DSLType::DSLType(skstd::string_view name, DSLModifiers* modifiers, PositionInfo position)
-        : fSkSLType(find_type(name, modifiers.fModifiers, position)) {}
+        : fSkSLType(find_type(name, &modifiers->fModifiers, position)) {}
 
 bool DSLType::isBoolean() const {
     return this->skslType().isBoolean();

src/sksl/ir/SkSLType.cpp

@@ -472,13 +472,13 @@ CoercionCost Type::coercionCost(const Type& other) const {
 }
 
 const Type* Type::applyPrecisionQualifiers(const Context& context,
-                                           const Modifiers& modifiers,
+                                           Modifiers* modifiers,
                                            SymbolTable* symbols,
                                            int line) const {
     // SkSL doesn't support low precision, so `lowp` is interpreted as medium precision.
-    bool highp   = modifiers.fFlags & Modifiers::kHighp_Flag;
+    bool highp   = modifiers->fFlags & Modifiers::kHighp_Flag;
-    bool mediump = modifiers.fFlags & Modifiers::kMediump_Flag;
+    bool mediump = modifiers->fFlags & Modifiers::kMediump_Flag;
-    bool lowp    = modifiers.fFlags & Modifiers::kLowp_Flag;
+    bool lowp    = modifiers->fFlags & Modifiers::kLowp_Flag;
 
     if (!lowp && !mediump && !highp) {
         // No precision qualifiers here. Return the type as-is.
@@ -497,6 +497,11 @@ const Type* Type::applyPrecisionQualifiers(const Context& context,
         return nullptr;
     }
 
+    // We're going to return a whole new type, so the modifier bits can be cleared out.
+    modifiers->fFlags &= ~(Modifiers::kHighp_Flag |
+                           Modifiers::kMediump_Flag |
+                           Modifiers::kLowp_Flag);
+
     const Type& component = this->componentType();
     if (component.highPrecision()) {
         if (highp) {

src/sksl/ir/SkSLType.h

@@ -532,7 +532,7 @@ public:
      * don't make sense, e.g. `highp bool` or `mediump MyStruct`.
      */
     const Type* applyPrecisionQualifiers(const Context& context,
-                                         const Modifiers& modifiers,
+                                         Modifiers* modifiers,
                                          SymbolTable* symbols,
                                          int line) const;
 

tests/SkSLTest.cpp

@@ -266,6 +266,7 @@ SKSL_TEST_ES3(SkSLIntrinsicUintBitsToFloat,    "intrinsics/UintBitsToFloat.sksl"
 SKSL_TEST_ES3(SkSLArrayNarrowingConversions,   "runtime/ArrayNarrowingConversions.rts")
 SKSL_TEST_ES3(SkSLLoopFloat,                   "runtime/LoopFloat.rts")
 SKSL_TEST_ES3(SkSLLoopInt,                     "runtime/LoopInt.rts")
+SKSL_TEST(SkSLPrecisionQualifiers,             "runtime/PrecisionQualifiers.rts")
 
 SKSL_TEST_ES3(SkSLArrayComparison,             "shared/ArrayComparison.sksl")
 SKSL_TEST_ES3(SkSLArrayConstructors,           "shared/ArrayConstructors.sksl")

tests/sksl/runtime/PrecisionQualifiers.stage

@@ -3,6 +3,9 @@ uniform half4 colorRed;
 bool test_vector_0();
 bool test_matrix_0();
 bool test_array_0();
+bool highp_param_0(float value);
+bool mediump_param_0(half value);
+bool lowp_param_0(half value);
 bool test_vector_0()
 {
 	half2 mp2 = half2(2.0);
@@ -43,6 +46,18 @@ bool test_array_0()
 	hv[1] = float2(2.0, 3.0);
 	return (float(mf[0]) == hf[0] && hv[0] == float2(mv[0])) && float2(mv[1]) == hv[1];
 }
+bool highp_param_0(float value)
+{
+	return value == 1.0;
+}
+bool mediump_param_0(half value)
+{
+	return value == 2.0;
+}
+bool lowp_param_0(half value)
+{
+	return value == 3.0;
+}
 float4 main(float2 coords)
 {
 	float4 zero = float4(0.0);
@@ -55,5 +70,5 @@ float4 main(float2 coords)
 	float _1_hp = float(_0_mp);
 	int _2_ihp = 2;
 	short _3_imp = short(_2_ihp);
-	return half4((((float(_0_mp) == _1_hp && _2_ihp == int(_3_imp)) && test_vector_0()) && test_matrix_0()) && test_array_0() ? float4(green) : red);
+	return half4(((((((float(_0_mp) == _1_hp && _2_ihp == int(_3_imp)) && test_vector_0()) && test_matrix_0()) && test_array_0()) && highp_param_0(1.0)) && mediump_param_0(2.0)) && lowp_param_0(3.0) ? float4(green) : red);
 }