Skip to content

Commit

Permalink
Revert "Create ContextUtils for packing and unpacking RP contexts."
Browse files Browse the repository at this point in the history
This reverts commit 11d3660.

Reason for revert: ASAN

Original change's description:
> Create ContextUtils for packing and unpacking RP contexts.
>
> Some upcoming changes to SkRP will allow us to generate many more
> ops with only 8-byte contexts (by replacing pointers with relative
> offsets into program data). In a 64-bit build, these 8-byte contexts
> are small enough to fit directly in the context pointer of a raster
> pipeline stage. (In a 32-bit build, we will need to continue using
> allocs, but this only applies to a dwindling population of very old
> devices which are unlikely to be keeping current on patches anyway.)
>
> SkRPCtxUtils::Pack will check if the passed-in struct is small enough
> to fit directly in the context field. If so, it will return the data
> bit-casted into a void pointer. If not, it allocates a copy of the
> struct inside the alloc and then returns a pointer to the copy.
>
> SkRPCtxUtils::Unpack performs the reverse operation: either
> un-bitcasting the object back to its original form, or returning the
> pointer as-is, depending on the size of the type.
>
> In followup CLs, we will be able to remove allocs for many common
> operations on 64-bit clients, by converting pointers into 32-bit
> offsets, and using Pack and Unpack when accessing the context.
> This should save memory, and makes execution more efficient on
> 64-bit clients by getting rid of a memory access.
>
> Change-Id: I8fae6ba8142aa5eec1ae446b3c3f0f16d8e2bb42
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/668697
> Commit-Queue: John Stiles <[email protected]>
> Auto-Submit: John Stiles <[email protected]>
> Reviewed-by: Michael Ludwig <[email protected]>

Change-Id: Ibaf80d7913eca2fc12e12b04e4322753cfecfc2d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/670037
Auto-Submit: John Stiles <[email protected]>
Commit-Queue: Rubber Stamper <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
  • Loading branch information
johnstiles-google authored and SkCQ committed Apr 12, 2023
1 parent b590baf commit 60777d3
Show file tree
Hide file tree
Showing 5 changed files with 0 additions and 122 deletions.
1 change: 0 additions & 1 deletion gn/core.gni
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,6 @@ skia_core_sources = [
"$_src/core/SkRasterPipeline.cpp",
"$_src/core/SkRasterPipeline.h",
"$_src/core/SkRasterPipelineBlitter.cpp",
"$_src/core/SkRasterPipelineContextUtils.h",
"$_src/core/SkRasterPipelineOpContexts.h",
"$_src/core/SkRasterPipelineOpList.h",
"$_src/core/SkReadBuffer.cpp",
Expand Down
1 change: 0 additions & 1 deletion public.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,6 @@ BASE_SRCS_ALL = [
"src/core/SkRasterPipeline.cpp",
"src/core/SkRasterPipeline.h",
"src/core/SkRasterPipelineBlitter.cpp",
"src/core/SkRasterPipelineContextUtils.h",
"src/core/SkRasterPipelineOpContexts.h",
"src/core/SkRasterPipelineOpList.h",
"src/core/SkReadBuffer.cpp",
Expand Down
1 change: 0 additions & 1 deletion src/core/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,6 @@ CORE_FILES = [
"SkRasterPipeline.cpp",
"SkRasterPipeline.h",
"SkRasterPipelineBlitter.cpp",
"SkRasterPipelineContextUtils.h",
"SkRasterPipelineOpContexts.h",
"SkRasterPipelineOpList.h",
"SkReadBuffer.cpp",
Expand Down
55 changes: 0 additions & 55 deletions src/core/SkRasterPipelineContextUtils.h

This file was deleted.

64 changes: 0 additions & 64 deletions tests/SkRasterPipelineTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include "src/base/SkUtils.h"
#include "src/core/SkOpts.h"
#include "src/core/SkRasterPipeline.h"
#include "src/core/SkRasterPipelineContextUtils.h"
#include "src/gpu/Swizzle.h"
#include "src/sksl/tracing/SkSLTraceHook.h"
#include "tests/Test.h"
Expand Down Expand Up @@ -45,69 +44,6 @@ DEF_TEST(SkRasterPipeline, r) {
REPORTER_ASSERT(r, ((result >> 48) & 0xffff) == 0x3c00);
}

DEF_TEST(SkRasterPipeline_PackSmallContext, r) {
struct PackableObject {
std::array<uint8_t, sizeof(void*)> data;
};

// Create an arena with storage.
using StorageArray = std::array<char, 128>;
StorageArray storage = {};
SkArenaAlloc alloc(storage.data(), storage.size(), 500);

StorageArray storageWhenEmpty = storage;

// Construct and pack one PackableObject.
PackableObject object;
std::fill(object.data.begin(), object.data.end(), 123);

const void* packed = SkRPCtxUtils::Pack(object, &alloc);

// The alloc's storage should still be the same as when it was empty.
REPORTER_ASSERT(r, storage == storageWhenEmpty);

// `packed` should now contain a bitwise cast of the raw object data.
uintptr_t objectBits = sk_bit_cast<uintptr_t>(packed);
for (size_t index = 0; index < sizeof(void*); ++index) {
REPORTER_ASSERT(r, (objectBits & 0xFF) == 123);
objectBits >>= 8;
}

// Now unpack it.
auto unpacked = SkRPCtxUtils::Unpack((const PackableObject*)packed);

// The data should be identical to the original.
REPORTER_ASSERT(r, unpacked.data == object.data);
}

DEF_TEST(SkRasterPipeline_PackBigContext, r) {
struct BigObject {
std::array<uint8_t, sizeof(void*) + 1> data;
};

// Create an arena with storage.
using StorageArray = std::array<char, 128>;
StorageArray storage = {};
SkArenaAlloc alloc(storage.data(), storage.size(), 500);

StorageArray storageWhenEmpty = storage;

// Construct and pack one BigObject.
BigObject object;
std::fill(object.data.begin(), object.data.end(), 123);

const void* packed = SkRPCtxUtils::Pack(object, &alloc);

// The alloc should have used its storage buffer; check that storage has changed somehow.
REPORTER_ASSERT(r, storage != storageWhenEmpty);

// Now unpack it.
auto unpacked = SkRPCtxUtils::Unpack((const BigObject*)packed);

// The data should be identical to the original.
REPORTER_ASSERT(r, unpacked.data == object.data);
}

DEF_TEST(SkRasterPipeline_LoadStoreConditionMask, r) {
alignas(64) int32_t mask[] = {~0, 0, ~0, 0, ~0, ~0, ~0, 0};
alignas(64) int32_t maskCopy[SkRasterPipeline_kMaxStride_highp] = {};
Expand Down

0 comments on commit 60777d3

Please sign in to comment.