Convert IRGenerator::convertPrefixExpr to PrefixExpression::Make.
Surprisingly, this actually improved our error detection slightly.
The expression `- -half4(0)` can now be simplified to `half4(0)` at
IR generation time, which allows the constant-folder to detect a
constant zero (and from there, a division by constant zero).
Change-Id: I8c4f6ab522efab5bf98913f9c6a1487b7af39a99
Bug: skia:11342, skia:11343
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/376842
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
diff --git a/src/sksl/SkSLIRGenerator.cpp b/src/sksl/SkSLIRGenerator.cpp
index a0e0ae2..2b12e43 100644
--- a/src/sksl/SkSLIRGenerator.cpp
+++ b/src/sksl/SkSLIRGenerator.cpp
@@ -2129,99 +2129,7 @@
if (!base) {
return nullptr;
}
- return this->convertPrefixExpression(expression.getOperator(), std::move(base));
-}
-
-std::unique_ptr<Expression> IRGenerator::convertPrefixExpression(Operator op,
- std::unique_ptr<Expression> base) {
- const Type& baseType = base->type();
- switch (op.kind()) {
- case Token::Kind::TK_PLUS:
- if (!baseType.componentType().isNumber()) {
- this->errorReporter().error(
- base->fOffset, "'+' cannot operate on '" + baseType.displayName() + "'");
- return nullptr;
- }
- return base;
-
- case Token::Kind::TK_MINUS:
- if (!baseType.componentType().isNumber()) {
- this->errorReporter().error(
- base->fOffset, "'-' cannot operate on '" + baseType.displayName() + "'");
- return nullptr;
- }
- if (base->is<IntLiteral>()) {
- return std::make_unique<IntLiteral>(base->fOffset,
- -base->as<IntLiteral>().value(),
- &base->type());
- }
- if (base->is<FloatLiteral>()) {
- return std::make_unique<FloatLiteral>(base->fOffset,
- -base->as<FloatLiteral>().value(),
- &base->type());
- }
- return std::make_unique<PrefixExpression>(Token::Kind::TK_MINUS, std::move(base));
-
- case Token::Kind::TK_PLUSPLUS:
- if (!baseType.isNumber()) {
- this->errorReporter().error(base->fOffset,
- String("'") + op.operatorName() +
- "' cannot operate on '" + baseType.displayName() + "'");
- return nullptr;
- }
- if (!Analysis::MakeAssignmentExpr(base.get(), VariableReference::RefKind::kReadWrite,
- &fContext.fErrors)) {
- return nullptr;
- }
- break;
- case Token::Kind::TK_MINUSMINUS:
- if (!baseType.isNumber()) {
- this->errorReporter().error(base->fOffset,
- String("'") + op.operatorName() +
- "' cannot operate on '" + baseType.displayName() + "'");
- return nullptr;
- }
- if (!Analysis::MakeAssignmentExpr(base.get(), VariableReference::RefKind::kReadWrite,
- &fContext.fErrors)) {
- return nullptr;
- }
- break;
- case Token::Kind::TK_LOGICALNOT:
- if (!baseType.isBoolean()) {
- this->errorReporter().error(base->fOffset,
- String("'") + op.operatorName() +
- "' cannot operate on '" + baseType.displayName() + "'");
- return nullptr;
- }
- if (base->is<BoolLiteral>()) {
- return std::make_unique<BoolLiteral>(base->fOffset,
- !base->as<BoolLiteral>().value(),
- &base->type());
- }
- break;
- case Token::Kind::TK_BITWISENOT:
- if (this->strictES2Mode()) {
- // GLSL ES 1.00, Section 5.1
- this->errorReporter().error(
- base->fOffset,
- String("operator '") + op.operatorName() + "' is not allowed");
- return nullptr;
- }
- if (!baseType.isInteger()) {
- this->errorReporter().error(base->fOffset,
- String("'") + op.operatorName() +
- "' cannot operate on '" + baseType.displayName() + "'");
- return nullptr;
- }
- if (baseType.isLiteral()) {
- // The expression `~123` is no longer a literal; coerce to the actual type.
- base = this->coerce(std::move(base), baseType.scalarTypeForLiteral());
- }
- break;
- default:
- SK_ABORT("unsupported prefix operator\n");
- }
- return std::make_unique<PrefixExpression>(op, std::move(base));
+ return PrefixExpression::Make(fContext, expression.getOperator(), std::move(base));
}
std::unique_ptr<Expression> IRGenerator::convertField(std::unique_ptr<Expression> base,
diff --git a/src/sksl/SkSLIRGenerator.h b/src/sksl/SkSLIRGenerator.h
index 06dcc59..00ff1e4 100644
--- a/src/sksl/SkSLIRGenerator.h
+++ b/src/sksl/SkSLIRGenerator.h
@@ -224,8 +224,6 @@
std::unique_ptr<Expression> convertPostfixExpression(std::unique_ptr<Expression> base,
Operator op);
std::unique_ptr<Expression> convertPostfixExpression(const ASTNode& expression);
- std::unique_ptr<Expression> convertPrefixExpression(Operator op,
- std::unique_ptr<Expression> base);
std::unique_ptr<Expression> convertScopeExpression(const ASTNode& expression);
std::unique_ptr<StructDefinition> convertStructDefinition(const ASTNode& expression);
std::unique_ptr<Expression> convertTypeField(int offset, const Type& type,
diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp
index 7899ca4..4ba9157 100644
--- a/src/sksl/SkSLInliner.cpp
+++ b/src/sksl/SkSLInliner.cpp
@@ -363,7 +363,7 @@
}
case Expression::Kind::kPrefix: {
const PrefixExpression& p = expression.as<PrefixExpression>();
- return std::make_unique<PrefixExpression>(p.getOperator(), expr(p.operand()));
+ return PrefixExpression::Make(*fContext, p.getOperator(), expr(p.operand()));
}
case Expression::Kind::kPostfix: {
const PostfixExpression& p = expression.as<PostfixExpression>();
diff --git a/src/sksl/SkSLRehydrator.cpp b/src/sksl/SkSLRehydrator.cpp
index 95d9f5d..225ecee 100644
--- a/src/sksl/SkSLRehydrator.cpp
+++ b/src/sksl/SkSLRehydrator.cpp
@@ -508,7 +508,7 @@
case Rehydrator::kPrefix_Command: {
Token::Kind op = (Token::Kind) this->readU8();
std::unique_ptr<Expression> operand = this->expression();
- return std::make_unique<PrefixExpression>(op, std::move(operand));
+ return PrefixExpression::Make(fContext, op, std::move(operand));
}
case Rehydrator::kSetting_Command: {
StringFragment name = this->readString();
diff --git a/src/sksl/dsl/priv/DSLWriter.cpp b/src/sksl/dsl/priv/DSLWriter.cpp
index 62307b7..f94e941 100644
--- a/src/sksl/dsl/priv/DSLWriter.cpp
+++ b/src/sksl/dsl/priv/DSLWriter.cpp
@@ -16,6 +16,7 @@
#include "src/sksl/dsl/DSLCore.h"
#include "src/sksl/dsl/DSLErrorHandling.h"
#include "src/sksl/ir/SkSLConstructor.h"
+#include "src/sksl/ir/SkSLPrefixExpression.h"
#include "src/sksl/ir/SkSLSwitchStatement.h"
#if !SKSL_USE_THREAD_LOCAL
@@ -136,7 +137,7 @@
std::unique_ptr<SkSL::Expression> DSLWriter::ConvertPrefix(Operator op,
std::unique_ptr<Expression> expr) {
- return IRGenerator().convertPrefixExpression(op, std::move(expr));
+ return PrefixExpression::Make(Context(), op, std::move(expr));
}
DSLPossibleStatement DSLWriter::ConvertSwitch(std::unique_ptr<Expression> value,
diff --git a/src/sksl/ir/SkSLPrefixExpression.cpp b/src/sksl/ir/SkSLPrefixExpression.cpp
index 62b537c..8e99779 100644
--- a/src/sksl/ir/SkSLPrefixExpression.cpp
+++ b/src/sksl/ir/SkSLPrefixExpression.cpp
@@ -14,25 +14,143 @@
namespace SkSL {
-static std::unique_ptr<Expression> negate_operand(const Expression& operand) {
- switch (operand.kind()) {
+static std::unique_ptr<Expression> negate_operand(const Context& context,
+ std::unique_ptr<Expression> operand) {
+ switch (operand->kind()) {
case Expression::Kind::kFloatLiteral:
// Convert -floatLiteral(1) to floatLiteral(-1).
- return std::make_unique<FloatLiteral>(operand.fOffset,
- -operand.as<FloatLiteral>().value(),
- &operand.type());
+ return std::make_unique<FloatLiteral>(operand->fOffset,
+ -operand->as<FloatLiteral>().value(),
+ &operand->type());
case Expression::Kind::kIntLiteral:
// Convert -intLiteral(1) to intLiteral(-1).
- return std::make_unique<IntLiteral>(operand.fOffset,
- -operand.as<IntLiteral>().value(),
- &operand.type());
+ return std::make_unique<IntLiteral>(operand->fOffset,
+ -operand->as<IntLiteral>().value(),
+ &operand->type());
+
+ case Expression::Kind::kConstructor:
+ // To be consistent with prior behavior, the conversion of a negated constructor into a
+ // constructor of negative values is only performed when optimization is on.
+ // Conceptually it's pretty similar to the int/float optimizations above, though.
+ if (context.fConfig->fSettings.fOptimize && operand->isCompileTimeConstant()) {
+ Constructor& ctor = operand->as<Constructor>();
+
+ // We've found a negated constant constructor, e.g.:
+ // -float4(float3(floatLiteral(1)), floatLiteral(2))
+ // To optimize this, the outer negation is removed and each argument is negated:
+ // float4(-float3(floatLiteral(1)), floatLiteral(-2))
+ // Recursion will continue to push negation inwards as deeply as possible:
+ // float4(float3(floatLiteral(-1)), floatLiteral(-2))
+ ExpressionArray args;
+ args.reserve_back(ctor.arguments().size());
+ for (std::unique_ptr<Expression>& arg : ctor.arguments()) {
+ args.push_back(negate_operand(context, std::move(arg)));
+ }
+ return Constructor::Make(context, ctor.fOffset, ctor.type(), std::move(args));
+ }
+ break;
default:
- // Convert Expr to Prefix(TK_MINUS, Expr).
- return std::make_unique<PrefixExpression>(Token::Kind::TK_MINUS,
- operand.clone());
+ break;
}
+
+ // No simplified form; convert expression to Prefix(TK_MINUS, expression).
+ return std::make_unique<PrefixExpression>(Token::Kind::TK_MINUS, std::move(operand));
+}
+
+static std::unique_ptr<Expression> logical_not_operand(const Context& context,
+ std::unique_ptr<Expression> operand) {
+ if (operand->is<BoolLiteral>()) {
+ // Convert !boolLiteral(true) to boolLiteral(false).
+ const BoolLiteral& b = operand->as<BoolLiteral>();
+ return std::make_unique<BoolLiteral>(operand->fOffset, !b.value(), &operand->type());
+ }
+
+ // No simplified form; convert expression to Prefix(TK_LOGICALNOT, expression).
+ return std::make_unique<PrefixExpression>(Token::Kind::TK_LOGICALNOT, std::move(operand));
+}
+
+std::unique_ptr<Expression> PrefixExpression::Make(const Context& context, Operator op,
+ std::unique_ptr<Expression> base) {
+ const Type& baseType = base->type();
+ switch (op.kind()) {
+ case Token::Kind::TK_PLUS:
+ if (!baseType.componentType().isNumber()) {
+ context.fErrors.error(base->fOffset,
+ "'+' cannot operate on '" + baseType.displayName() + "'");
+ return nullptr;
+ }
+ return base;
+
+ case Token::Kind::TK_MINUS:
+ if (!baseType.componentType().isNumber()) {
+ context.fErrors.error(base->fOffset,
+ "'-' cannot operate on '" + baseType.displayName() + "'");
+ return nullptr;
+ }
+ return negate_operand(context, std::move(base));
+
+ case Token::Kind::TK_PLUSPLUS:
+ if (!baseType.isNumber()) {
+ context.fErrors.error(base->fOffset,
+ String("'") + op.operatorName() + "' cannot operate on '" +
+ baseType.displayName() + "'");
+ return nullptr;
+ }
+ if (!Analysis::MakeAssignmentExpr(base.get(), VariableReference::RefKind::kReadWrite,
+ &context.fErrors)) {
+ return nullptr;
+ }
+ break;
+
+ case Token::Kind::TK_MINUSMINUS:
+ if (!baseType.isNumber()) {
+ context.fErrors.error(base->fOffset,
+ String("'") + op.operatorName() + "' cannot operate on '" +
+ baseType.displayName() + "'");
+ return nullptr;
+ }
+ if (!Analysis::MakeAssignmentExpr(base.get(), VariableReference::RefKind::kReadWrite,
+ &context.fErrors)) {
+ return nullptr;
+ }
+ break;
+
+ case Token::Kind::TK_LOGICALNOT:
+ if (!baseType.isBoolean()) {
+ context.fErrors.error(base->fOffset,
+ String("'") + op.operatorName() + "' cannot operate on '" +
+ baseType.displayName() + "'");
+ return nullptr;
+ }
+ return logical_not_operand(context, std::move(base));
+
+ case Token::Kind::TK_BITWISENOT:
+ if (context.fConfig->strictES2Mode()) {
+ // GLSL ES 1.00, Section 5.1
+ context.fErrors.error(
+ base->fOffset,
+ String("operator '") + op.operatorName() + "' is not allowed");
+ return nullptr;
+ }
+ if (!baseType.isInteger()) {
+ context.fErrors.error(base->fOffset,
+ String("'") + op.operatorName() + "' cannot operate on '" +
+ baseType.displayName() + "'");
+ return nullptr;
+ }
+ if (baseType.isLiteral()) {
+ // The expression `~123` is no longer a literal; coerce to the actual type.
+ base = baseType.scalarTypeForLiteral().coerceExpression(std::move(base), context);
+ }
+ break;
+
+ default:
+ SK_ABORT("unsupported prefix operator\n");
+ }
+
+ return std::make_unique<PrefixExpression>(op, std::move(base));
}
std::unique_ptr<Expression> PrefixExpression::constantPropagate(const IRGenerator& irGenerator,
@@ -43,33 +161,15 @@
switch (this->operand()->kind()) {
case Expression::Kind::kFloatLiteral:
case Expression::Kind::kIntLiteral:
- return negate_operand(*this->operand());
-
- case Expression::Kind::kConstructor: {
- // We've found a negated constant vector, e.g.:
- // -float4(float3(floatLiteral(1)), floatLiteral(2))
- // To optimize this, the outer negation is removed and each argument is negated:
- // float4(-float3(floatLiteral(1)), floatLiteral(-2))
- // Steady state is reached after another optimization pass:
- // float4(float3(floatLiteral(-1)), floatLiteral(-2))
- const Constructor& constructor = this->operand()->as<Constructor>();
- ExpressionArray args;
- args.reserve_back(constructor.arguments().size());
- for (const std::unique_ptr<Expression>& arg : constructor.arguments()) {
- args.push_back(negate_operand(*arg));
- }
- return Constructor::Make(irGenerator.fContext, constructor.fOffset,
- constructor.type(), std::move(args));
- }
+ case Expression::Kind::kConstructor:
+ return negate_operand(irGenerator.fContext, this->operand()->clone());
default:
break;
}
} else if (this->getOperator().kind() == Token::Kind::TK_LOGICALNOT) {
if (this->operand()->is<BoolLiteral>()) {
- // Convert !boolLiteral(true) to boolLiteral(false).
- const BoolLiteral& b = this->operand()->as<BoolLiteral>();
- return std::make_unique<BoolLiteral>(b.fOffset, !b.value(), &b.type());
+ return logical_not_operand(irGenerator.fContext, this->operand()->clone());
}
}
}
diff --git a/src/sksl/ir/SkSLPrefixExpression.h b/src/sksl/ir/SkSLPrefixExpression.h
index 673c789..2f40ef4 100644
--- a/src/sksl/ir/SkSLPrefixExpression.h
+++ b/src/sksl/ir/SkSLPrefixExpression.h
@@ -13,6 +13,8 @@
#include "src/sksl/SkSLOperators.h"
#include "src/sksl/ir/SkSLExpression.h"
+#include <memory>
+
namespace SkSL {
/**
@@ -22,11 +24,15 @@
public:
static constexpr Kind kExpressionKind = Kind::kPrefix;
+ // Use PrefixExpression::Make to automatically simplify various prefix expression types.
PrefixExpression(Operator op, std::unique_ptr<Expression> operand)
: INHERITED(operand->fOffset, kExpressionKind, &operand->type())
, fOperator(op)
, fOperand(std::move(operand)) {}
+ static std::unique_ptr<Expression> Make(const Context& context, Operator op,
+ std::unique_ptr<Expression> base);
+
Operator getOperator() const {
return fOperator;
}
@@ -52,8 +58,7 @@
const DefinitionMap& definitions) override;
std::unique_ptr<Expression> clone() const override {
- return std::unique_ptr<Expression>(new PrefixExpression(this->getOperator(),
- this->operand()->clone()));
+ return std::make_unique<PrefixExpression>(this->getOperator(), this->operand()->clone());
}
String description() const override {
diff --git a/tests/sksl/errors/Ossfuzz27663.glsl b/tests/sksl/errors/Ossfuzz27663.glsl
index b68394b..8cffafe 100644
--- a/tests/sksl/errors/Ossfuzz27663.glsl
+++ b/tests/sksl/errors/Ossfuzz27663.glsl
@@ -1,4 +1,5 @@
### Compilation failed:
+error: 1: division by zero
error: 1: type mismatch: '=' cannot operate on 'half4', 'float4'
-1 error
+2 errors
