Rework stable function calls.
Trying to use magic offset values ended up causing issues in
other parts of the code, which rightly assume that positions are
bounded within the program text.
Now we have a stable pointer, which always tracks `this` except
in cases of `clone` where it preserves the cloned call's value.
This is simpler and more directly maps to the problem I am
trying to solve.
Bug: b/328051548
Change-Id: I5606959dafd8e0d1dfe81e935a0fa2cc53f611f2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/883096
Commit-Queue: Nathan Sanchez <nathanasanchez@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Nathan Sanchez <nathanasanchez@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
diff --git a/src/sksl/SkSLInliner.cpp b/src/sksl/SkSLInliner.cpp
index 83e271d..b323b8d 100644
--- a/src/sksl/SkSLInliner.cpp
+++ b/src/sksl/SkSLInliner.cpp
@@ -16,7 +16,6 @@
#include "src/sksl/SkSLAnalysis.h"
#include "src/sksl/SkSLDefines.h"
#include "src/sksl/SkSLErrorReporter.h"
-#include "src/sksl/SkSLModule.h"
#include "src/sksl/SkSLOperator.h"
#include "src/sksl/SkSLPosition.h"
#include "src/sksl/analysis/SkSLProgramUsage.h"
@@ -302,22 +301,11 @@
}
case Expression::Kind::kFunctionCall: {
const FunctionCall& funcCall = expression.as<FunctionCall>();
-
- // We need to calculate a new stable ID here, since each function call in a program is
- // required to have a unique stable ID. Start from the largest possible value and work
- // backwards. We use the `unknown` ModuleType since this shouldn't conflict with any
- // real-world code.
- int syntheticOffset = Position::kMaxOffset - fInlinedFunctionCallCounter;
- ++fInlinedFunctionCallCounter;
- Position syntheticPos = Position::Range(syntheticOffset, syntheticOffset);
-
- return FunctionCall::Make(
- *fContext,
- pos,
- funcCall.type().clone(*fContext, symbolTableForExpression),
- funcCall.function(),
- argList(funcCall.arguments()),
- FunctionCall::MakeStableID(ModuleType::unknown, syntheticPos));
+ return FunctionCall::Make(*fContext,
+ pos,
+ funcCall.type().clone(*fContext, symbolTableForExpression),
+ funcCall.function(),
+ argList(funcCall.arguments()));
}
case Expression::Kind::kFunctionReference:
return expression.clone(pos);
diff --git a/src/sksl/SkSLInliner.h b/src/sksl/SkSLInliner.h
index f602af7..68fd318 100644
--- a/src/sksl/SkSLInliner.h
+++ b/src/sksl/SkSLInliner.h
@@ -115,7 +115,6 @@
const Context* fContext = nullptr;
Mangler fMangler;
int fInlinedStatementCounter = 0;
- int fInlinedFunctionCallCounter = 0;
};
} // namespace SkSL
diff --git a/src/sksl/analysis/SkSLSpecialization.cpp b/src/sksl/analysis/SkSLSpecialization.cpp
index fc41b29..25122af 100644
--- a/src/sksl/analysis/SkSLSpecialization.cpp
+++ b/src/sksl/analysis/SkSLSpecialization.cpp
@@ -105,7 +105,8 @@
// variables to specialize on.
if (specialization.count() > 0) {
Specializations& specializations = fSpecializationMap[&decl];
- SpecializedCallKey callKey{call.stableID(), fInheritedSpecializationIndex};
+ SpecializedCallKey callKey{call.stablePointer(),
+ fInheritedSpecializationIndex};
for (int i = 0; i < specializations.size(); i++) {
const SpecializedParameters& entry = specializations[i];
@@ -161,7 +162,7 @@
SpecializationIndex FindSpecializationIndexForCall(const FunctionCall& call,
const SpecializationInfo& info,
SpecializationIndex parentSpecializationIndex) {
- SpecializedCallKey callKey{call.stableID(), parentSpecializationIndex};
+ SpecializedCallKey callKey{call.stablePointer(), parentSpecializationIndex};
SpecializationIndex* foundIndex = info.fSpecializedCallMap.find(callKey);
return foundIndex ? *foundIndex : kUnspecialized;
}
diff --git a/src/sksl/analysis/SkSLSpecialization.h b/src/sksl/analysis/SkSLSpecialization.h
index 6d86fd5..c3b1ea2 100644
--- a/src/sksl/analysis/SkSLSpecialization.h
+++ b/src/sksl/analysis/SkSLSpecialization.h
@@ -14,7 +14,6 @@
#include "src/utils/SkBitSet.h"
#include <cstddef>
-#include <cstdint>
#include <functional>
namespace SkSL {
@@ -64,17 +63,17 @@
struct SpecializedCallKey {
struct Hash {
size_t operator()(const SpecializedCallKey& entry) {
- return SkGoodHash()(entry.fStableID) ^
+ return SkGoodHash()(entry.fStablePointer) ^
SkGoodHash()(entry.fParentSpecializationIndex);
}
};
bool operator==(const SpecializedCallKey& other) const {
- return fStableID == other.fStableID &&
+ return fStablePointer == other.fStablePointer &&
fParentSpecializationIndex == other.fParentSpecializationIndex;
}
- const uint32_t fStableID = 0;
+ const FunctionCall* fStablePointer = nullptr;
SpecializationIndex fParentSpecializationIndex = Analysis::kUnspecialized;
};
diff --git a/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp b/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp
index 8fab419..dd7e6fc 100644
--- a/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp
+++ b/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp
@@ -25,7 +25,6 @@
#include "src/sksl/SkSLErrorReporter.h"
#include "src/sksl/SkSLIntrinsicList.h"
#include "src/sksl/SkSLMemoryLayout.h"
-#include "src/sksl/SkSLModule.h"
#include "src/sksl/SkSLOperator.h"
#include "src/sksl/SkSLOutputStream.h"
#include "src/sksl/SkSLPool.h"
@@ -5117,9 +5116,8 @@
args.push_back(ConstructorCompound::MakeFromConstants(fContext, Position{},
*fContext.fTypes.fFloat2, kZero));
}
- uint32_t callMainStableID = FunctionCall::MakeStableID(ModuleType::unknown, Position());
auto callMainFn = FunctionCall::Make(fContext, Position(), &main.returnType(),
- main, std::move(args), callMainStableID);
+ main, std::move(args));
// Synthesize `skFragColor = main()` as a BinaryExpression.
auto assignmentStmt = std::make_unique<ExpressionStatement>(std::make_unique<BinaryExpression>(
diff --git a/src/sksl/ir/SkSLFunctionCall.cpp b/src/sksl/ir/SkSLFunctionCall.cpp
index 1a554ad..33b6fda 100644
--- a/src/sksl/ir/SkSLFunctionCall.cpp
+++ b/src/sksl/ir/SkSLFunctionCall.cpp
@@ -1004,7 +1004,7 @@
std::unique_ptr<Expression> FunctionCall::clone(Position pos) const {
return std::make_unique<FunctionCall>(pos, &this->type(), &this->function(),
- this->arguments().clone(), this->stableID());
+ this->arguments().clone(), this->stablePointer());
}
std::string FunctionCall::description(OperatorPrecedence) const {
@@ -1238,26 +1238,14 @@
return ChildCall::Make(context, pos, returnType, child, std::move(arguments));
}
- return Make(context, pos, returnType, function, std::move(arguments),
- MakeStableID(context.fConfig->fModuleType, pos));
-}
-
-uint32_t FunctionCall::MakeStableID(ModuleType moduleType, Position pos) {
- // Synthesize a stable ID from the function-call position and module type.
- uint32_t stableID = pos.valid() ? pos.startOffset() : 0x00FFFFFF;
- SkASSERT(stableID < 0x01000000);
-
- stableID |= SkToU32(moduleType) << 24;
-
- return stableID;
+ return Make(context, pos, returnType, function, std::move(arguments));
}
std::unique_ptr<Expression> FunctionCall::Make(const Context& context,
Position pos,
const Type* returnType,
const FunctionDeclaration& function,
- ExpressionArray arguments,
- uint32_t stableID) {
+ ExpressionArray arguments) {
SkASSERT(function.parameters().size() == SkToSizeT(arguments.size()));
// We might be able to optimize built-in intrinsics.
@@ -1274,7 +1262,7 @@
}
return std::make_unique<FunctionCall>(pos, returnType, &function, std::move(arguments),
- stableID);
+ /*stablePointer=*/nullptr);
}
} // namespace SkSL
diff --git a/src/sksl/ir/SkSLFunctionCall.h b/src/sksl/ir/SkSLFunctionCall.h
index 0d7c6a4..eae8e01 100644
--- a/src/sksl/ir/SkSLFunctionCall.h
+++ b/src/sksl/ir/SkSLFunctionCall.h
@@ -23,7 +23,6 @@
class Context;
class FunctionDeclaration;
class Type;
-enum class ModuleType : int8_t;
enum class OperatorPrecedence : uint8_t;
/**
@@ -33,12 +32,15 @@
public:
inline static constexpr Kind kIRNodeKind = Kind::kFunctionCall;
- FunctionCall(Position pos, const Type* type, const FunctionDeclaration* function,
- ExpressionArray arguments, uint32_t stableID)
- : INHERITED(pos, kIRNodeKind, type)
- , fFunction(*function)
- , fArguments(std::move(arguments))
- , fStableID(stableID) {}
+ FunctionCall(Position pos,
+ const Type* type,
+ const FunctionDeclaration* function,
+ ExpressionArray arguments,
+ const FunctionCall* stablePointer)
+ : INHERITED(pos, kIRNodeKind, type)
+ , fFunction(*function)
+ , fArguments(std::move(arguments))
+ , fStablePointer(stablePointer ? stablePointer : this) {}
// Resolves generic types, performs type conversion on arguments, determines return type, and
// chooses a unique stable ID. Reports errors via the ErrorReporter.
@@ -57,16 +59,12 @@
Position pos,
const Type* returnType,
const FunctionDeclaration& function,
- ExpressionArray arguments,
- uint32_t stableID);
+ ExpressionArray arguments);
static const FunctionDeclaration* FindBestFunctionForCall(const Context& context,
const FunctionDeclaration* overloads,
const ExpressionArray& arguments);
- // Given a module type and an offset into the code, returns a stable ID.
- static uint32_t MakeStableID(ModuleType moduleType, Position pos);
-
const FunctionDeclaration& function() const {
return fFunction;
}
@@ -79,8 +77,8 @@
return fArguments;
}
- uint32_t stableID() const {
- return fStableID;
+ const FunctionCall* stablePointer() const {
+ return fStablePointer;
}
std::unique_ptr<Expression> clone(Position pos) const override;
@@ -91,9 +89,9 @@
const FunctionDeclaration& fFunction;
ExpressionArray fArguments;
- // The stable ID is a 32-bit value which uniquely identifies this FunctionCall across an entire
- // SkSL program. It is preserved across calls to Clone() or Make(), unlike a pointer address.
- uint32_t fStableID;
+ // The stable pointer uniquely identifies this FunctionCall across an entire SkSL program.
+ // This allows us to clone() a FunctionCall but still find that call in a hash-map.
+ const FunctionCall* fStablePointer = nullptr;
using INHERITED = Expression;
};
