Skip to content

Commit

Permalink
Revert "Use packed contexts for copy/splat-constant ops in SkRP."
Browse files Browse the repository at this point in the history 
This reverts commit 8554018.

Reason for revert: ASAN

Original change's description:
> Use packed contexts for copy/splat-constant ops in SkRP.
>
> This means that ops which write a constant value to a slot no longer
> need to rely on a separate alloc on 64-bit CPUs. The RP stage's
> context pointer is large enough to store the data we need directly,
> in packed form.
>
> Change-Id: I04564ea56ee211667c4328559c0052c42b2df527
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/667959
> Reviewed-by: Herb Derby <[email protected]>
> Auto-Submit: John Stiles <[email protected]>
> Commit-Queue: John Stiles <[email protected]>

Change-Id: I12568b9cc685c8a76ba87413eac74c08f1ad15c0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/670036
Auto-Submit: John Stiles <[email protected]>
Commit-Queue: Rubber Stamper <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
  • Loading branch information
@johnstiles-google
johnstiles-google authored and SkCQ committed Apr 12, 2023
1 parent 92905a7 commit b590baf
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 52 deletions.
4 changes: 1 addition & 3 deletions src/core/SkRasterPipelineOpContexts.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,8 @@ struct SkRasterPipeline_TablesCtx {
const uint8_t *r, *g, *b, *a;
};

using SkRPOffset = uint32_t;

struct SkRasterPipeline_ConstantCtx {
SkRPOffset dst;
float *dst;
float value;
};

Expand Down
33 changes: 13 additions & 20 deletions src/opts/SkRasterPipeline_opts.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,8 @@
#include "modules/skcms/skcms.h"
#include "src/base/SkUtils.h" // unaligned_{load,store}
#include "src/core/SkRasterPipeline.h"
#include "src/core/SkRasterPipelineContextUtils.h"
#include "src/sksl/tracing/SkSLTraceHook.h"

#include <cstdint>
#include <type_traits>

// Every function in this file should be marked static and inline using SI.
#if defined(__clang__)
Expand Down Expand Up @@ -1349,7 +1346,7 @@ static void start_pipeline(size_t dx, size_t dy,
sk_unaligned_store(ctx->dg, dg);
sk_unaligned_store(ctx->db, db);
sk_unaligned_store(ctx->da, da);
ctx->base = base;
ctx->base = ctx->base;
ctx->stage = program;
}
#endif
Expand Down Expand Up @@ -3518,28 +3515,24 @@ STAGE_TAIL(copy_4_uniforms, SkRasterPipeline_UniformCtx* ctx) {
dst[3] = src[3];
}

STAGE_TAIL(copy_constant, SkRasterPipeline_ConstantCtx* packed) {
auto ctx = SkRPCtxUtils::Unpack(packed);
F* dst = (F*)(base + ctx.dst);
F value = ctx.value;
STAGE_TAIL(copy_constant, SkRasterPipeline_ConstantCtx* ctx) {
F* dst = (F*)ctx->dst;
F value = ctx->value;
dst[0] = value;
}
STAGE_TAIL(splat_2_constants, SkRasterPipeline_ConstantCtx* packed) {
auto ctx = SkRPCtxUtils::Unpack(packed);
F* dst = (F*)(base + ctx.dst);
F value = ctx.value;
STAGE_TAIL(splat_2_constants, SkRasterPipeline_ConstantCtx* ctx) {
F* dst = (F*)ctx->dst;
F value = ctx->value;
dst[0] = dst[1] = value;
}
STAGE_TAIL(splat_3_constants, SkRasterPipeline_ConstantCtx* packed) {
auto ctx = SkRPCtxUtils::Unpack(packed);
F* dst = (F*)(base + ctx.dst);
F value = ctx.value;
STAGE_TAIL(splat_3_constants, SkRasterPipeline_ConstantCtx* ctx) {
F* dst = (F*)ctx->dst;
F value = ctx->value;
dst[0] = dst[1] = dst[2] = value;
}
STAGE_TAIL(splat_4_constants, SkRasterPipeline_ConstantCtx* packed) {
auto ctx = SkRPCtxUtils::Unpack(packed);
F* dst = (F*)(base + ctx.dst);
F value = ctx.value;
STAGE_TAIL(splat_4_constants, SkRasterPipeline_ConstantCtx* ctx) {
F* dst = (F*)ctx->dst;
F value = ctx->value;
dst[0] = dst[1] = dst[2] = dst[3] = value;
}

Expand Down
34 changes: 11 additions & 23 deletions src/sksl/codegen/SkSLRasterPipelineBuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
#include "include/sksl/SkSLPosition.h"
#include "src/base/SkArenaAlloc.h"
#include "src/core/SkOpts.h"
#include "src/core/SkRasterPipelineContextUtils.h"
#include "src/core/SkRasterPipelineOpContexts.h"
#include "src/core/SkRasterPipelineOpList.h"
#include "src/core/SkTHash.h"
Expand All @@ -27,7 +26,6 @@

#include <algorithm>
#include <cmath>
#include <cstddef>
#include <cstring>
#include <iterator>
#include <string>
Expand Down Expand Up @@ -1462,10 +1460,6 @@ void Program::makeStages(TArray<Stage>* pipeline,
}
};

auto OffsetFromBase = [&](const void* ptr) -> SkRPOffset {
return (SkRPOffset)((std::byte*)ptr - (std::byte*)slots.values.data());
};

// Write each BuilderOp to the pipeline array.
pipeline->reserve_back(fInstructions.size());
for (const Instruction& inst : fInstructions) {
Expand Down Expand Up @@ -1793,15 +1787,14 @@ void Program::makeStages(TArray<Stage>* pipeline,
} else {
// Splat constant values onto the stack.
for (int remaining = inst.fImmA; remaining > 0; remaining -= 4) {
SkRasterPipeline_ConstantCtx ctx;
ctx.dst = OffsetFromBase(dst);
ctx.value = sk_bit_cast<float>(inst.fImmB);
void* ptr = SkRPCtxUtils::Pack(ctx, alloc);
auto ctx = alloc->make<SkRasterPipeline_ConstantCtx>();
ctx->dst = dst;
ctx->value = sk_bit_cast<float>(inst.fImmB);
switch (remaining) {
case 1: pipeline->push_back({ProgramOp::copy_constant, ptr}); break;
case 2: pipeline->push_back({ProgramOp::splat_2_constants,ptr}); break;
case 3: pipeline->push_back({ProgramOp::splat_3_constants,ptr}); break;
default: pipeline->push_back({ProgramOp::splat_4_constants,ptr}); break;
case 1: pipeline->push_back({ProgramOp::copy_constant, ctx}); break;
case 2: pipeline->push_back({ProgramOp::splat_2_constants,ctx}); break;
case 3: pipeline->push_back({ProgramOp::splat_3_constants,ctx}); break;
default: pipeline->push_back({ProgramOp::splat_4_constants,ctx}); break;
}
dst += 4 * N;
}
Expand Down Expand Up @@ -2197,11 +2190,6 @@ void Program::dump(SkWStream* out) const {
return "ExternalPtr(" + AsRange(0, numSlots) + ")";
};

// Interprets a slab offset as a slot range.
auto OffsetCtx = [&](SkRPOffset offset, int numSlots) -> std::string {
return PtrCtx((std::byte*)slots.values.data() + offset, numSlots);
};

// Interpret the context value as a pointer to two adjacent values.
auto AdjacentPtrCtx = [&](const void* ctx,
int numSlots) -> std::tuple<std::string, std::string> {
Expand Down Expand Up @@ -2325,11 +2313,11 @@ void Program::dump(SkWStream* out) const {
return std::make_tuple(dst, src);
};

// Interpret the context value as a packed ConstantCtx structure.
// Interpret the context value as a ConstantCtx structure.
auto ConstantCtx = [&](const void* v, int slots) -> std::tuple<std::string, std::string> {
auto ctx = SkRPCtxUtils::Unpack((const SkRasterPipeline_ConstantCtx*)v);
return std::make_tuple(OffsetCtx(ctx.dst, slots),
Imm(ctx.value));
const auto* ctx = static_cast<const SkRasterPipeline_ConstantCtx*>(v);
return std::make_tuple(PtrCtx(ctx->dst, slots),
Imm(ctx->value));
};

std::string opArg1, opArg2, opArg3, opSwizzle;
Expand Down
11 changes: 5 additions & 6 deletions tests/SkRasterPipelineTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1303,11 +1303,10 @@ DEF_TEST(SkRasterPipeline_CopyConstant, r) {
// Overwrite one destination slot with a constant (1000 + the slot number).
SkArenaAlloc alloc(/*firstHeapAllocation=*/256);
SkRasterPipeline p(&alloc);
SkRasterPipeline_ConstantCtx ctx;
ctx.dst = N * index * sizeof(float);
ctx.value = 1000.0f + index;
p.append(SkRasterPipelineOp::set_base_pointer, &slots[0]);
p.append(SkRasterPipelineOp::copy_constant, SkRPCtxUtils::Pack(ctx, &alloc));
auto* ctx = alloc.make<SkRasterPipeline_ConstantCtx>();
ctx->dst = &slots[N * index];
ctx->value = 1000.0f + index;
p.append(SkRasterPipelineOp::copy_constant, ctx);
p.run(0,0,1,1);

// Verify that our constant value has been broadcast into exactly one slot.
Expand All @@ -1316,7 +1315,7 @@ DEF_TEST(SkRasterPipeline_CopyConstant, r) {
for (int checkSlot = 0; checkSlot < 5; ++checkSlot) {
for (int checkLane = 0; checkLane < N; ++checkLane) {
if (checkSlot == index) {
REPORTER_ASSERT(r, *destPtr == ctx.value);
REPORTER_ASSERT(r, *destPtr == ctx->value);
} else {
REPORTER_ASSERT(r, *destPtr == expectedUnchanged);
}
Expand Down

0 comments on commit b590baf

Please sign in to comment.