more bits for shader program hash
gm/lumafilter is drawing wrong for me with SkVMBlitter,
but only when I enable its cache. I thought it must be
a collision between program fingerprints, but on further
inspection that doesn't seem to be the case, even 32-bit.
But now that I've done this, might as well keep it.
Even if p(collision) is small, p^2 is smaller.
Other small cleanups to what we hash and how:
- don't include derived fields in Instruction equality and hashing
- use SkOpts::hash() instead of manual std::hash/xor hack
Small struct layout changes to keep everything dense.
Change-Id: I5ba817296f0bfefa0e18f62d103094d0c63bd50d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/266282
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
diff --git a/src/core/SkVM.cpp b/src/core/SkVM.cpp
index 87c7a1b..912d4cd 100644
--- a/src/core/SkVM.cpp
+++ b/src/core/SkVM.cpp
@@ -13,8 +13,8 @@
#include "include/private/SkThreadID.h"
#include "include/private/SkVx.h"
#include "src/core/SkCpu.h"
+#include "src/core/SkOpts.h"
#include "src/core/SkVM.h"
-#include <functional> // std::hash
bool gSkVMJITViaDylib{false};
@@ -446,39 +446,22 @@
return {fProgram, fStrides, debug_name};
}
- // TODO: it's probably not important that we include post-Builder::done() fields like
- // death, can_hoist, and used_in_loop in operator==() and InstructionHash::operator().
- // They'll always have the same, initial values as set in Builder::push().
-
+ // We skip fields only written after Builder::done() (death, can_hoist, used_in_loop) here
+ // for equality and hashing. They'll always have the same default values in Builder::push().
static bool operator==(const Builder::Instruction& a, const Builder::Instruction& b) {
- return a.op == b.op
- && a.x == b.x
- && a.y == b.y
- && a.z == b.z
- && a.immy == b.immy
- && a.immz == b.immz
- && a.death == b.death
- && a.can_hoist == b.can_hoist
- && a.used_in_loop == b.used_in_loop;
+ return a.op == b.op
+ && a.x == b.x
+ && a.y == b.y
+ && a.z == b.z
+ && a.immy == b.immy
+ && a.immz == b.immz;
}
- // TODO: replace with SkOpts::hash()?
- size_t Builder::InstructionHash::operator()(const Instruction& inst) const {
- auto hash = [](auto v) {
- return std::hash<decltype(v)>{}(v);
- };
- return hash((uint8_t)inst.op)
- ^ hash(inst.x)
- ^ hash(inst.y)
- ^ hash(inst.z)
- ^ hash(inst.immy)
- ^ hash(inst.immz)
- ^ hash(inst.death)
- ^ hash(inst.can_hoist)
- ^ hash(inst.used_in_loop);
+ uint32_t Builder::InstructionHash::operator()(const Instruction& inst, uint32_t seed) const {
+ return SkOpts::hash(&inst, offsetof(Instruction, death), seed);
}
- uint32_t Builder::hash() const { return fHash; }
+ uint64_t Builder::hash() const { return (uint64_t)fHashLo | (uint64_t)fHashHi << 32; }
// Most instructions produce a value and return it by ID,
// the value-producing instruction's own index in the program vector.
@@ -486,9 +469,11 @@
Instruction inst{op, x, y, z, immy, immz,
/*death=*/0, /*can_hoist=*/true, /*used_in_loop=*/false};
- // This InstructionHash{}() call should be free given we're about to use fIndex below.
- fHash ^= InstructionHash{}(inst);
- fHash = SkChecksum::CheapMix(fHash); // Make sure instruction order matters.
+ // This first InstructionHash{}() call should be free given we're about to use fIndex below.
+ fHashLo ^= InstructionHash{}(inst, 0); // Two hash streams with different seeds.
+ fHashHi ^= InstructionHash{}(inst, 1);
+ fHashLo = SkChecksum::CheapMix(fHashLo); // Mix to make sure instruction order matters.
+ fHashHi = SkChecksum::CheapMix(fHashHi);
// Basic common subexpression elimination:
// if we've already seen this exact Instruction, use it instead of creating a new one.
diff --git a/src/core/SkVM.h b/src/core/SkVM.h
index 2ddd335..84780b5 100644
--- a/src/core/SkVM.h
+++ b/src/core/SkVM.h
@@ -319,7 +319,7 @@
M(select) M(bytes) M(pack) \
// End of SKVM_OPS
- enum class Op : uint8_t {
+ enum class Op : int {
#define M(op) op,
SKVM_OPS(M)
#undef M
@@ -338,9 +338,11 @@
class Builder {
public:
struct Instruction {
+ // Tightly packed for hashing:
Op op; // v* = op(x,y,z,imm), where * == index of this Instruction.
Val x,y,z; // Enough arguments for mad().
int immy,immz; // Immediate bit pattern, shift count, argument index, etc.
+ // End tightly packed for hashing.
// Not populated until done() has been called.
int death; // Index of last live instruction taking this input; live if != 0.
@@ -539,11 +541,11 @@
void dump(SkWStream* = nullptr) const;
- uint32_t hash() const;
+ uint64_t hash() const;
private:
struct InstructionHash {
- size_t operator()(const Instruction& inst) const;
+ uint32_t operator()(const Instruction& inst, uint32_t seed=0) const;
};
Val push(Op, Val x, Val y=NA, Val z=NA, int immy=0, int immz=0);
@@ -562,7 +564,8 @@
SkTHashMap<Instruction, Val, InstructionHash> fIndex;
std::vector<Instruction> fProgram;
std::vector<int> fStrides;
- uint32_t fHash{0};
+ uint32_t fHashLo{0},
+ fHashHi{0};
};
// Helper to streamline allocating and working with uniforms.
diff --git a/src/core/SkVMBlitter.cpp b/src/core/SkVMBlitter.cpp
index 5248307..5f86e21 100644
--- a/src/core/SkVMBlitter.cpp
+++ b/src/core/SkVMBlitter.cpp
@@ -50,11 +50,12 @@
SK_BEGIN_REQUIRE_DENSE;
struct Key {
uint64_t colorSpace;
- uint32_t shader;
+ uint64_t shader;
uint8_t colorType,
alphaType,
blendMode,
coverage;
+ uint32_t padding{0};
// Params::quality and Params::ctm are only passed to shader->program(),
// not used here by the blitter itself. No need to include them in the key;
// they'll be folded into the shader key if used.
@@ -77,7 +78,7 @@
SK_END_REQUIRE_DENSE;
static SkString debug_name(const Key& key) {
- return SkStringPrintf("CT%d-AT%d-Cov%d-Blend%d-CS%llx-Shader%x",
+ return SkStringPrintf("CT%d-AT%d-Cov%d-Blend%d-CS%llx-Shader%llx",
key.colorType,
key.alphaType,
key.coverage,
@@ -106,7 +107,7 @@
// If Builder can't build this program, CacheKey() sets *ok to false.
static Key CacheKey(const Params& params, skvm::Uniforms* uniforms, bool* ok) {
SkASSERT(params.shader);
- uint32_t shaderHash = 0;
+ uint64_t shaderHash = 0;
{
const SkShaderBase* shader = as_SB(params.shader);
skvm::Builder p;
