From 0dd66fea56d3b9d6ff3f9e9fcdd9081a519b8e48 Mon Sep 17 00:00:00 2001 From: Ethan Nicholas Date: Fri, 25 Mar 2022 12:39:10 -0400 Subject: [PATCH] Added full position tracking to Swizzle No visible effect yet, but this will enable better error reporting in a future CL. Change-Id: I09e1c5d3bb423a7ce42701f15c4bb142b0a9473c Reviewed-on: https://skia-review.googlesource.com/c/skia/+/524638 Reviewed-by: Brian Salomon Reviewed-by: John Stiles Commit-Queue: Ethan Nicholas --- include/sksl/SkSLPosition.h | 10 +++++++ src/sksl/SkSLDSLParser.cpp | 9 ++++--- src/sksl/SkSLInliner.cpp | 2 +- src/sksl/SkSLRehydrator.cpp | 39 ++++++++++++---------------- src/sksl/dsl/DSLCore.cpp | 8 +++--- src/sksl/ir/SkSLBinaryExpression.cpp | 5 ++-- src/sksl/ir/SkSLIndexExpression.cpp | 5 ++-- src/sksl/ir/SkSLSwizzle.cpp | 27 +++++++++---------- src/sksl/ir/SkSLSwizzle.h | 16 +++++++----- 9 files changed, 66 insertions(+), 55 deletions(-) diff --git a/include/sksl/SkSLPosition.h b/include/sksl/SkSLPosition.h index e26954b432..3b3c9c8fcb 100644 --- a/include/sksl/SkSLPosition.h +++ b/include/sksl/SkSLPosition.h @@ -31,6 +31,7 @@ public: } static Position Range(int startOffset, int endOffset) { + SkASSERT(startOffset <= endOffset); Position result; result.fStartOffsetOrLine = startOffset; result.fEndOffset = endOffset; @@ -59,6 +60,15 @@ public: return fEndOffset; } + // Returns the position from this through, and including the entirety of, end. + Position rangeThrough(Position end) const { + if (fEndOffset == -1 || end.fEndOffset == -1) { + return *this; + } + SkASSERT(this->startOffset() <= end.startOffset() && this->endOffset() <= end.endOffset()); + return Range(this->startOffset(), end.endOffset()); + } + bool operator==(const Position& other) const { return fStartOffsetOrLine == other.fStartOffsetOrLine && fEndOffset == other.fEndOffset; diff --git a/src/sksl/SkSLDSLParser.cpp b/src/sksl/SkSLDSLParser.cpp index bc10ee3b3f..0f0e217bdf 100644 --- a/src/sksl/SkSLDSLParser.cpp +++ b/src/sksl/SkSLDSLParser.cpp @@ -1708,11 +1708,12 @@ DSLExpression DSLParser::swizzle(Position pos, DSLExpression base, } } switch (length) { - case 1: return dsl::Swizzle(std::move(base), components[0]); - case 2: return dsl::Swizzle(std::move(base), components[0], components[1]); - case 3: return dsl::Swizzle(std::move(base), components[0], components[1], components[2]); + case 1: return dsl::Swizzle(std::move(base), components[0], pos); + case 2: return dsl::Swizzle(std::move(base), components[0], components[1], pos); + case 3: return dsl::Swizzle(std::move(base), components[0], components[1], components[2], + pos); case 4: return dsl::Swizzle(std::move(base), components[0], components[1], components[2], - components[3]); + components[3], pos); default: SkUNREACHABLE; } } diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp index e5596333b6..12c95fcef3 100644 --- a/src/sksl/SkSLInliner.cpp +++ b/src/sksl/SkSLInliner.cpp @@ -434,7 +434,7 @@ std::unique_ptr Inliner::inlineExpression(Position pos, return expression.clone(); case Expression::Kind::kSwizzle: { const Swizzle& s = expression.as(); - return Swizzle::Make(*fContext, expr(s.base()), s.components()); + return Swizzle::Make(*fContext, pos, expr(s.base()), s.components()); } case Expression::Kind::kTernary: { const TernaryExpression& t = expression.as(); diff --git a/src/sksl/SkSLRehydrator.cpp b/src/sksl/SkSLRehydrator.cpp index 79f2c05454..004bd233e4 100644 --- a/src/sksl/SkSLRehydrator.cpp +++ b/src/sksl/SkSLRehydrator.cpp @@ -495,6 +495,7 @@ ExpressionArray Rehydrator::expressionArray() { } std::unique_ptr Rehydrator::expression() { + Position pos; int kind = this->readU8(); switch (kind) { case Rehydrator::kBinary_Command: { @@ -505,63 +506,55 @@ std::unique_ptr Rehydrator::expression() { } case Rehydrator::kBoolLiteral_Command: { bool value = this->readU8(); - return Literal::MakeBool(this->context(), Position(), value); + return Literal::MakeBool(this->context(), pos, value); } case Rehydrator::kConstructorArray_Command: { const Type* type = this->type(); - return ConstructorArray::Make(this->context(), Position(), *type, - this->expressionArray()); + return ConstructorArray::Make(this->context(), pos, *type, this->expressionArray()); } case Rehydrator::kConstructorArrayCast_Command: { const Type* type = this->type(); ExpressionArray args = this->expressionArray(); SkASSERT(args.size() == 1); - return ConstructorArrayCast::Make(this->context(), Position(), *type, - std::move(args[0])); + return ConstructorArrayCast::Make(this->context(), pos, *type, std::move(args[0])); } case Rehydrator::kConstructorCompound_Command: { const Type* type = this->type(); - return ConstructorCompound::Make(this->context(), Position(), *type, - this->expressionArray()); + return ConstructorCompound::Make(this->context(), pos, *type, this->expressionArray()); } case Rehydrator::kConstructorDiagonalMatrix_Command: { const Type* type = this->type(); ExpressionArray args = this->expressionArray(); SkASSERT(args.size() == 1); - return ConstructorDiagonalMatrix::Make(this->context(), Position(), *type, - std::move(args[0])); + return ConstructorDiagonalMatrix::Make(this->context(), pos, *type, std::move(args[0])); } case Rehydrator::kConstructorMatrixResize_Command: { const Type* type = this->type(); ExpressionArray args = this->expressionArray(); SkASSERT(args.size() == 1); - return ConstructorMatrixResize::Make(this->context(), Position(), *type, - std::move(args[0])); + return ConstructorMatrixResize::Make(this->context(), pos, *type, std::move(args[0])); } case Rehydrator::kConstructorScalarCast_Command: { const Type* type = this->type(); ExpressionArray args = this->expressionArray(); SkASSERT(args.size() == 1); - return ConstructorScalarCast::Make(this->context(), Position(), *type, - std::move(args[0])); + return ConstructorScalarCast::Make(this->context(), pos, *type, std::move(args[0])); } case Rehydrator::kConstructorSplat_Command: { const Type* type = this->type(); ExpressionArray args = this->expressionArray(); SkASSERT(args.size() == 1); - return ConstructorSplat::Make(this->context(), Position(), *type, std::move(args[0])); + return ConstructorSplat::Make(this->context(), pos, *type, std::move(args[0])); } case Rehydrator::kConstructorStruct_Command: { const Type* type = this->type(); - return ConstructorStruct::Make(this->context(), Position(), *type, - this->expressionArray()); + return ConstructorStruct::Make(this->context(), pos, *type, this->expressionArray()); } case Rehydrator::kConstructorCompoundCast_Command: { const Type* type = this->type(); ExpressionArray args = this->expressionArray(); SkASSERT(args.size() == 1); - return ConstructorCompoundCast::Make(this->context(), Position(), *type, - std::move(args[0])); + return ConstructorCompoundCast::Make(this->context(), pos, *type, std::move(args[0])); } case Rehydrator::kFieldAccess_Command: { std::unique_ptr base = this->expression(); @@ -574,7 +567,7 @@ std::unique_ptr Rehydrator::expression() { int32_t floatBits = this->readS32(); float value; memcpy(&value, &floatBits, sizeof(value)); - return Literal::MakeFloat(Position(), value, type); + return Literal::MakeFloat(pos, value, type); } case Rehydrator::kFunctionCall_Command: { const Type* type = this->type(); @@ -592,7 +585,7 @@ std::unique_ptr Rehydrator::expression() { SkASSERT(false); return nullptr; } - return FunctionCall::Make(this->context(), Position(), type, *f, std::move(args)); + return FunctionCall::Make(this->context(), pos, type, *f, std::move(args)); } case Rehydrator::kIndex_Command: { std::unique_ptr base = this->expression(); @@ -603,10 +596,10 @@ std::unique_ptr Rehydrator::expression() { const Type* type = this->type(); if (type->isUnsigned()) { unsigned int value = this->readU32(); - return Literal::MakeInt(Position(), value, type); + return Literal::MakeInt(pos, value, type); } else { int value = this->readS32(); - return Literal::MakeInt(Position(), value, type); + return Literal::MakeInt(pos, value, type); } } case Rehydrator::kPostfix_Command: { @@ -630,7 +623,7 @@ std::unique_ptr Rehydrator::expression() { for (int i = 0; i < count; ++i) { components.push_back(this->readU8()); } - return Swizzle::Make(this->context(), std::move(base), components); + return Swizzle::Make(this->context(), pos, std::move(base), components); } case Rehydrator::kTernary_Command: { std::unique_ptr test = this->expression(); diff --git a/src/sksl/dsl/DSLCore.cpp b/src/sksl/dsl/DSLCore.cpp index 2e41680016..e14cf03e49 100644 --- a/src/sksl/dsl/DSLCore.cpp +++ b/src/sksl/dsl/DSLCore.cpp @@ -314,7 +314,7 @@ public: static DSLExpression Swizzle(DSLExpression base, SkSL::SwizzleComponent::Type a, Position pos) { - return DSLExpression(Swizzle::Convert(ThreadContext::Context(), base.release(), + return DSLExpression(Swizzle::Convert(ThreadContext::Context(), pos, base.release(), ComponentArray{a}), pos); } @@ -323,7 +323,7 @@ public: SkSL::SwizzleComponent::Type a, SkSL::SwizzleComponent::Type b, Position pos) { - return DSLExpression(Swizzle::Convert(ThreadContext::Context(), base.release(), + return DSLExpression(Swizzle::Convert(ThreadContext::Context(), pos, base.release(), ComponentArray{a, b}), pos); } @@ -333,7 +333,7 @@ public: SkSL::SwizzleComponent::Type b, SkSL::SwizzleComponent::Type c, Position pos) { - return DSLExpression(Swizzle::Convert(ThreadContext::Context(), base.release(), + return DSLExpression(Swizzle::Convert(ThreadContext::Context(), pos, base.release(), ComponentArray{a, b, c}), pos); } @@ -344,7 +344,7 @@ public: SkSL::SwizzleComponent::Type c, SkSL::SwizzleComponent::Type d, Position pos) { - return DSLExpression(Swizzle::Convert(ThreadContext::Context(), base.release(), + return DSLExpression(Swizzle::Convert(ThreadContext::Context(), pos, base.release(), ComponentArray{a,b,c,d}), pos); } diff --git a/src/sksl/ir/SkSLBinaryExpression.cpp b/src/sksl/ir/SkSLBinaryExpression.cpp index 874c013fb4..40feb6f2d4 100644 --- a/src/sksl/ir/SkSLBinaryExpression.cpp +++ b/src/sksl/ir/SkSLBinaryExpression.cpp @@ -45,8 +45,9 @@ static std::unique_ptr rewrite_matrix_vector_multiply(const Context& std::unique_ptr matN = IndexExpression::Make( context, left.clone(), Literal::MakeInt(context, left.fPosition, n)); // Get vec[N] with a swizzle expression. - std::unique_ptr vecN = Swizzle::Make( - context, right.clone(), ComponentArray{(SkSL::SwizzleComponent::Type)n}); + std::unique_ptr vecN = Swizzle::Make(context, + left.fPosition.rangeThrough(right.fPosition), right.clone(), + ComponentArray{(SkSL::SwizzleComponent::Type)n}); // Multiply them together. const Type* matNType = &matN->type(); std::unique_ptr product = diff --git a/src/sksl/ir/SkSLIndexExpression.cpp b/src/sksl/ir/SkSLIndexExpression.cpp index ede5172e7a..1157059983 100644 --- a/src/sksl/ir/SkSLIndexExpression.cpp +++ b/src/sksl/ir/SkSLIndexExpression.cpp @@ -98,10 +98,12 @@ std::unique_ptr IndexExpression::Make(const Context& context, if (indexExpr->isIntLiteral()) { SKSL_INT indexValue = indexExpr->as().intValue(); if (!index_out_of_range(context, indexValue, *base)) { + Position pos = base->fPosition; if (baseType.isVector()) { // Constant array indexes on vectors can be converted to swizzles: `v[2]` --> `v.z`. // Swizzling is harmless and can unlock further simplifications for some base types. - return Swizzle::Make(context, std::move(base), ComponentArray{(int8_t)indexValue}); + return Swizzle::Make(context, pos, std::move(base), + ComponentArray{(int8_t)indexValue}); } if (baseType.isArray() && !base->hasSideEffects()) { @@ -143,7 +145,6 @@ std::unique_ptr IndexExpression::Make(const Context& context, } if (!ctorArgs.empty()) { - Position pos = ctorArgs.front()->fPosition; return ConstructorCompound::Make(context, pos, vecType, std::move(ctorArgs)); } } diff --git a/src/sksl/ir/SkSLSwizzle.cpp b/src/sksl/ir/SkSLSwizzle.cpp index fd843c9272..2b3c8ccad2 100644 --- a/src/sksl/ir/SkSLSwizzle.cpp +++ b/src/sksl/ir/SkSLSwizzle.cpp @@ -107,6 +107,7 @@ static std::string mask_string(const ComponentArray& components) { } static std::unique_ptr optimize_constructor_swizzle(const Context& context, + Position pos, const AnyConstructor& base, ComponentArray components) { auto baseArguments = base.argumentSpan(); @@ -220,19 +221,20 @@ static std::unique_ptr optimize_constructor_swizzle(const Context& c if (reorderedArg.fComponents.empty()) { newArgs.push_back(std::move(newArg)); } else { - newArgs.push_back(Swizzle::Make(context, std::move(newArg), + newArgs.push_back(Swizzle::Make(context, pos, std::move(newArg), reorderedArg.fComponents)); } } // Wrap the new argument list in a constructor. return Constructor::Convert(context, - base.fPosition, + pos, componentType.toCompound(context, swizzleSize, /*rows=*/1), std::move(newArgs)); } std::unique_ptr Swizzle::Convert(const Context& context, + Position pos, std::unique_ptr base, std::string_view maskString) { ComponentArray components; @@ -257,12 +259,12 @@ std::unique_ptr Swizzle::Convert(const Context& context, case 'q': components.push_back(SwizzleComponent::Q); break; case 'B': components.push_back(SwizzleComponent::UB); break; default: - context.fErrors->error(base->fPosition, + context.fErrors->error(pos, String::printf("invalid swizzle component '%c'", field)); return nullptr; } } - return Convert(context, std::move(base), std::move(components)); + return Convert(context, pos, std::move(base), std::move(components)); } // Swizzles are complicated due to constant components. The most difficult case is a mask like @@ -271,15 +273,14 @@ std::unique_ptr Swizzle::Convert(const Context& context, // secondary swizzle to put them back into the right order, so in this case we end up with // 'float4(base.xw, 1, 0).xzyw'. std::unique_ptr Swizzle::Convert(const Context& context, + Position pos, std::unique_ptr base, ComponentArray inComponents) { if (!validate_swizzle_domain(inComponents)) { - context.fErrors->error(base->fPosition, - "invalid swizzle mask '" + mask_string(inComponents) + "'"); + context.fErrors->error(pos, "invalid swizzle mask '" + mask_string(inComponents) + "'"); return nullptr; } - const Position pos = base->fPosition; const Type& baseType = base->type(); if (!baseType.isVector() && !baseType.isScalar()) { @@ -363,7 +364,7 @@ std::unique_ptr Swizzle::Convert(const Context& context, // scalar.x0x0 -> type2(scalar) // vector.zyx -> vector.zyx // vector.x0y0 -> vector.xy - std::unique_ptr expr = Swizzle::Make(context, std::move(base), maskComponents); + std::unique_ptr expr = Swizzle::Make(context, pos, std::move(base), maskComponents); // If we have processed the entire swizzle, we're done. if (maskComponents.count() == inComponents.count()) { @@ -428,10 +429,11 @@ std::unique_ptr Swizzle::Convert(const Context& context, return nullptr; } - return Swizzle::Make(context, std::move(expr), swizzleComponents); + return Swizzle::Make(context, pos, std::move(expr), swizzleComponents); } std::unique_ptr Swizzle::Make(const Context& context, + Position pos, std::unique_ptr expr, ComponentArray components) { const Type& exprType = expr->type(); @@ -450,7 +452,6 @@ std::unique_ptr Swizzle::Make(const Context& context, // SkSL supports splatting a scalar via `scalar.xxxx`, but not all versions of GLSL allow this. // Replace swizzles with equivalent splat constructors (`scalar.xxx` --> `half3(value)`). if (exprType.isScalar()) { - Position pos = expr->fPosition; return ConstructorSplat::Make(context, pos, exprType.toCompound(context, components.size(), /*rows=*/1), std::move(expr)); @@ -480,7 +481,7 @@ std::unique_ptr Swizzle::Make(const Context& context, // It may actually be possible to further simplify this swizzle. Go again. // (e.g. `color.abgr.abgr` --> `color.rgba` --> `color`.) - return Swizzle::Make(context, std::move(base.base()), combined); + return Swizzle::Make(context, pos, std::move(base.base()), combined); } // If we are swizzling a constant expression, we can use its value instead here (so that @@ -501,13 +502,13 @@ std::unique_ptr Swizzle::Make(const Context& context, // Optimize swizzles of constructors. if (value->isAnyConstructor()) { const AnyConstructor& ctor = value->asAnyConstructor(); - if (auto replacement = optimize_constructor_swizzle(context, ctor, components)) { + if (auto replacement = optimize_constructor_swizzle(context, pos, ctor, components)) { return replacement; } } // The swizzle could not be simplified, so apply the requested swizzle to the base expression. - return std::make_unique(context, std::move(expr), components); + return std::make_unique(context, pos, std::move(expr), components); } } // namespace SkSL diff --git a/src/sksl/ir/SkSLSwizzle.h b/src/sksl/ir/SkSLSwizzle.h index e1024fbfe6..2bdeebafe3 100644 --- a/src/sksl/ir/SkSLSwizzle.h +++ b/src/sksl/ir/SkSLSwizzle.h @@ -22,9 +22,9 @@ namespace SkSL { struct Swizzle final : public Expression { inline static constexpr Kind kExpressionKind = Kind::kSwizzle; - Swizzle(const Context& context, std::unique_ptr base, + Swizzle(const Context& context, Position pos, std::unique_ptr base, const ComponentArray& components) - : INHERITED(base->fPosition, kExpressionKind, + : INHERITED(pos, kExpressionKind, &base->type().componentType().toCompound(context, components.size(), 1)) , fBase(std::move(base)) , fComponents(components) { @@ -35,16 +35,19 @@ struct Swizzle final : public Expression { // errors via ErrorReporter, and returns an expression that combines constructors and native // swizzles (comprised solely of X/Y/W/Z). static std::unique_ptr Convert(const Context& context, + Position pos, std::unique_ptr base, ComponentArray inComponents); static std::unique_ptr Convert(const Context& context, + Position pos, std::unique_ptr base, std::string_view maskString); // Swizzle::Make does not permit ZERO or ONE in the component array, just X/Y/Z/W; errors are // reported via ASSERT. static std::unique_ptr Make(const Context& context, + Position pos, std::unique_ptr expr, ComponentArray inComponents); @@ -65,8 +68,8 @@ struct Swizzle final : public Expression { } std::unique_ptr clone() const override { - return std::unique_ptr(new Swizzle(&this->type(), this->base()->clone(), - this->components())); + return std::unique_ptr(new Swizzle(fPosition, &this->type(), + this->base()->clone(), this->components())); } std::string description() const override { @@ -78,8 +81,9 @@ struct Swizzle final : public Expression { } private: - Swizzle(const Type* type, std::unique_ptr base, const ComponentArray& components) - : INHERITED(base->fPosition, kExpressionKind, type) + Swizzle(Position pos, const Type* type, std::unique_ptr base, + const ComponentArray& components) + : INHERITED(pos, kExpressionKind, type) , fBase(std::move(base)) , fComponents(components) { SkASSERT(this->components().size() >= 1 && this->components().size() <= 4);