Elide sample coords for GrSkSLFP if only used for pass-through sampling
If every use of the coords passed to main was for a call to sample that
was converted to pass-through sampling... We don't actually reference
the sample coords, so we can un-set the flag. This prevents the FP from
requesting an extra set of (unused) coords, which saves a varying
between the vertex and fragment shaders.
Bug: skia:11869
Change-Id: I4e15876031717b8bcf642e742bad8ae26d6bd020
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/411871
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
diff --git a/src/core/SkRuntimeEffect.cpp b/src/core/SkRuntimeEffect.cpp
index 7dba94c..e6b2dd9 100644
--- a/src/core/SkRuntimeEffect.cpp
+++ b/src/core/SkRuntimeEffect.cpp
@@ -206,6 +206,7 @@
std::vector<Uniform> uniforms;
std::vector<Child> children;
std::vector<SkSL::SampleUsage> sampleUsages;
+ int elidedSampleCoords = 0;
const SkSL::Context& ctx(compiler->context());
// Go through program elements, pulling out information that we need
@@ -226,7 +227,7 @@
c.index = children.size();
children.push_back(c);
sampleUsages.push_back(SkSL::Analysis::GetSampleUsage(
- *program, var, sampleCoordsUsage.fWrite != 0));
+ *program, var, sampleCoordsUsage.fWrite != 0, &elidedSampleCoords));
}
// 'uniform' variables
else if (var.modifiers().fFlags & SkSL::Modifiers::kUniform_Flag) {
@@ -259,6 +260,14 @@
}
}
+ // If the sample coords are never written to, then we will have converted sample calls that use
+ // them unmodified into "passthrough" sampling. If all references to the sample coords were of
+ // that form, then we don't actually "use" sample coords. We unset the flag to prevent creating
+ // an extra (unused) varying holding the coords.
+ if (elidedSampleCoords == sampleCoordsUsage.fRead && sampleCoordsUsage.fWrite == 0) {
+ flags &= ~kUsesSampleCoords_Flag;
+ }
+
#undef RETURN_FAILURE
sk_sp<SkRuntimeEffect> effect(new SkRuntimeEffect(std::move(sksl),
diff --git a/src/gpu/effects/GrSkSLFP.cpp b/src/gpu/effects/GrSkSLFP.cpp
index 3ce35ab..077f553 100644
--- a/src/gpu/effects/GrSkSLFP.cpp
+++ b/src/gpu/effects/GrSkSLFP.cpp
@@ -135,11 +135,11 @@
args.fFragBuilder->declareGlobal(inputColorCopy);
args.fFragBuilder->codeAppendf("%s = %s;\n", inputColorCopy.c_str(), args.fInputColor);
- // Callback to define a function (and return its mangled name)
+ // Copy the incoming coords to a local variable. Code in main might modify the coords
+ // parameter. fSampleCoord could be a varying, so writes to it would be illegal.
SkString coordsVarName = args.fFragBuilder->newTmpVarName("coords");
- const char* coords = nullptr;
+ const char* coords = coordsVarName.c_str();
if (fp.referencesSampleCoords()) {
- coords = coordsVarName.c_str();
args.fFragBuilder->codeAppendf("float2 %s = %s;\n", coords, args.fSampleCoord);
}
diff --git a/src/sksl/SkSLAnalysis.cpp b/src/sksl/SkSLAnalysis.cpp
index ab819a6..665f561 100644
--- a/src/sksl/SkSLAnalysis.cpp
+++ b/src/sksl/SkSLAnalysis.cpp
@@ -83,11 +83,14 @@
return fUsage;
}
+ int elidedSampleCoordCount() const { return fElidedSampleCoordCount; }
+
protected:
const Context& fContext;
const Variable& fFP;
const bool fWritesToSampleCoords;
SampleUsage fUsage;
+ int fElidedSampleCoordCount = 0;
bool visitExpression(const Expression& e) override {
// Looking for sample(fp, ...)
@@ -107,6 +110,7 @@
->modifiers()
.fLayout.fBuiltin == SK_MAIN_COORDS_BUILTIN) {
fUsage.merge(SampleUsage::PassThrough());
+ ++fElidedSampleCoordCount;
} else {
fUsage.merge(SampleUsage::Explicit());
}
@@ -565,9 +569,14 @@
SampleUsage Analysis::GetSampleUsage(const Program& program,
const Variable& fp,
- bool writesToSampleCoords) {
+ bool writesToSampleCoords,
+ int* elidedSampleCoordCount) {
MergeSampleUsageVisitor visitor(*program.fContext, fp, writesToSampleCoords);
- return visitor.visit(program);
+ SampleUsage result = visitor.visit(program);
+ if (elidedSampleCoordCount) {
+ *elidedSampleCoordCount += visitor.elidedSampleCoordCount();
+ }
+ return result;
}
bool Analysis::ReferencesBuiltin(const Program& program, int builtin) {
diff --git a/src/sksl/SkSLAnalysis.h b/src/sksl/SkSLAnalysis.h
index ee61671..937bee4 100644
--- a/src/sksl/SkSLAnalysis.h
+++ b/src/sksl/SkSLAnalysis.h
@@ -37,10 +37,13 @@
* Determines how `program` samples `fp`. By default, assumes that the sample coords
* (SK_MAIN_COORDS_BUILTIN) might be modified, so `sample(fp, sampleCoords)` is treated as
* Explicit. If writesToSampleCoords is false, treats that as PassThrough, instead.
+ * If elidedSampleCoordCount is provided, the pointed to value will be incremented by the
+ * number of sample calls where the above rewrite was performed.
*/
static SampleUsage GetSampleUsage(const Program& program,
const Variable& fp,
- bool writesToSampleCoords = true);
+ bool writesToSampleCoords = true,
+ int* elidedSampleCoordCount = nullptr);
static bool ReferencesBuiltin(const Program& program, int builtin);
diff --git a/tests/SkRuntimeEffectTest.cpp b/tests/SkRuntimeEffectTest.cpp
index 399d530..e28a2d9 100644
--- a/tests/SkRuntimeEffectTest.cpp
+++ b/tests/SkRuntimeEffectTest.cpp
@@ -586,11 +586,6 @@
//
// It also checks that we correctly set the "referencesSampleCoords" bit on the runtime effect
// FP, depending on how the coords parameter to main is used.
- //
- // TODO(skia:11869): Today, any reference to sample coords marks them as being referenced. If
- // the *only* use is in sample calls that are converted to passthrough by our optimization,
- // they should not be considered referenced. (This will prevent us from adding an unnecessary
- // varying to the shaders).
auto test = [&](const char* src, bool expectExplicit, bool expectReferencesSampleCoords) {
auto [effect, err] =
@@ -607,9 +602,9 @@
// Cases where our optimization is valid, and works:
- // Direct use of passed-in coords
- // TODO(skia:11869): This is the case where referencesSampleCoords *should* be false
- test("half4 main(float2 xy) { return sample(child, xy); }", false, true);
+ // Direct use of passed-in coords. Here, the only use of sample coords is for a sample call
+ // converted to passthrough, so referenceSampleCoords is *false*, despite appearing in main.
+ test("half4 main(float2 xy) { return sample(child, xy); }", false, false);
// Sample with passed-in coords, read (but don't write) sample coords elsewhere
test("half4 main(float2 xy) { return sample(child, xy) + sin(xy.x); }", false, true);