Reland "Use tailcall recursion to process skcms ops." This reverts commit e0a502ae9dab9b659018cbc07d98dda20a305cec. Reason for revert: experiment complete - https://screenshot.googleplex.com/3jGGaL8NX3kthzn Original change's description: > Revert "Use tailcall recursion to process skcms ops." > > This reverts commit 7cf575c421865b23b898e7a1b9cbcd3f6ecc63da. > > Reason for revert: This is a temporary revert to test for performance deltas on Pinpoint. Barring unforeseen circumstances, this CL will be relanded shortly. > > Original change's description: > > Use tailcall recursion to process skcms ops. > > > > This is dramatically faster on M1 processors (and likely other ARMs) > > compared to a giant switch. It is also 25% faster on AMD EPYC (hsw). > > The MSVS + clang-cl configuration fails tests when SKCMS_MUSTTAIL is > > used, so SKCMS_MUSTTAIL is disabled in this configuration; I don't > > have access to a Windows machine to check the difference in actual > > code generation, but hopefully this is a negligible difference. > > > > Change-Id: Ie2f9569d19f3804fcf3ba77b6148fe8f5c8e13f9 > > Bug: b/305974160 > > Reviewed-on: https://skia-review.googlesource.com/c/skcms/+/771479 > > Commit-Queue: John Stiles <johnstiles@google.com> > > Reviewed-by: Brian Osman <brianosman@google.com> > > Bug: b/305974160 > Change-Id: I03728c1e9be80c4e8d5a0ae808fb2c977420efab > Reviewed-on: https://skia-review.googlesource.com/c/skcms/+/784541 > Reviewed-by: Brian Osman <brianosman@google.com> > Commit-Queue: John Stiles <johnstiles@google.com> > Auto-Submit: John Stiles <johnstiles@google.com> Bug: b/305974160 Change-Id: I1a97be6d27ae9ea719c0d8308e3f6b52ab09efa2 Reviewed-on: https://skia-review.googlesource.com/c/skcms/+/784990 Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Commit-Queue: John Stiles <johnstiles@google.com>
diff --git a/src/Transform_inl.h b/src/Transform_inl.h index 5b3fe49..3920cea 100644 --- a/src/Transform_inl.h +++ b/src/Transform_inl.h
@@ -769,27 +769,40 @@ template <typename T> operator T*() { return (const T*)fArg; } }; -#define DECLARE_STAGE(name, arg) \ - SI void Exec_##name##_k(arg, const char* src, char* dst, F& r, F& g, F& b, F& a, int i); \ - \ - SI void Exec_##name(const void* v, const char* s, char* d, F& r, F& g, F& b, F& a, int i) { \ - Exec_##name##_k(Ctx{v}, s, d, r, g, b, a, i); \ - } \ - \ - SI void Exec_##name##_k(arg, \ - SKCMS_MAYBE_UNUSED const char* src, \ - SKCMS_MAYBE_UNUSED char* dst, \ - SKCMS_MAYBE_UNUSED F& r, \ - SKCMS_MAYBE_UNUSED F& g, \ - SKCMS_MAYBE_UNUSED F& b, \ - SKCMS_MAYBE_UNUSED F& a, \ +// We can't declare StageFn as a function pointer which takes a pointer to StageFns; that would be +// a circular dependency. To avoid this, StageFn is wrapped in a `struct StageList` which forward- +// declare here. +struct StageList; +using StageFn = void (*)(StageList stages, const void** ctx, const char* s, char* d, + F r, F g, F b, F a, int i); +struct StageList { + const StageFn* fn; +}; + +#define DECLARE_STAGE(name, arg, CALL_NEXT) \ + SI void Exec_##name##_k(arg, const char* src, char* dst, F& r, F& g, F& b, F& a, int i); \ + \ + SI void Exec_##name(StageList list, const void** ctx, const char* s, char* d, \ + F r, F g, F b, F a, int i) { \ + Exec_##name##_k(Ctx{*ctx}, s, d, r, g, b, a, i); \ + ++list.fn; ++ctx; \ + CALL_NEXT; \ + } \ + \ + SI void Exec_##name##_k(arg, \ + SKCMS_MAYBE_UNUSED const char* src, \ + SKCMS_MAYBE_UNUSED char* dst, \ + SKCMS_MAYBE_UNUSED F& r, \ + SKCMS_MAYBE_UNUSED F& g, \ + SKCMS_MAYBE_UNUSED F& b, \ + SKCMS_MAYBE_UNUSED F& a, \ SKCMS_MAYBE_UNUSED int i) #define STAGE(name, arg) \ - DECLARE_STAGE(name, arg) + DECLARE_STAGE(name, arg, SKCMS_MUSTTAIL return (*list.fn)(list, ctx, s, d, r, g, b, a, i)) #define FINAL_STAGE(name, arg) \ - DECLARE_STAGE(name, arg) + DECLARE_STAGE(name, arg, /*just return to exec_stages*/) STAGE(load_a8, NoCtx) { a = F_from_U8(load<U8>(src + 1*i)); @@ -1195,7 +1208,7 @@ clut(b2a, &r,&g,&b,&a); } -// From here on down, the store_ ops are all "final stages," ending the loop. +// From here on down, the store_ ops are all "final stages," terminating the tail-call recursion. FINAL_STAGE(store_a8, NoCtx) { store(dst + 1*i, cast<U8>(to_fixed(a * 255))); @@ -1433,29 +1446,31 @@ #endif } -static void exec_ops(const Op* ops, const void** contexts, - const char* src, char* dst, int i) { - F r = F0, g = F0, b = F0, a = F1; - while (true) { - switch (*ops++) { -#define M(name) case Op::name: Exec_##name(*contexts++, src, dst, r, g, b, a, i); break; - SKCMS_LOAD_OPS(M) - SKCMS_WORK_OPS(M) -#undef M -#define M(name) case Op::name: Exec_##name(*contexts++, src, dst, r, g, b, a, i); return; - SKCMS_STORE_OPS(M) -#undef M - } - } +SI void exec_stages(StageList list, const void** contexts, const char* src, char* dst, int i) { + (*list.fn)(list, contexts, src, dst, F0, F0, F0, F1, i); } // NOLINTNEXTLINE(misc-definitions-in-headers) -void run_program(const Op* program, const void** contexts, ptrdiff_t /*programSize*/, +void run_program(const Op* program, const void** contexts, ptrdiff_t programSize, const char* src, char* dst, int n, const size_t src_bpp, const size_t dst_bpp) { + // Convert the program into an array of tailcall stages. + StageFn stages[32]; + assert(programSize <= ARRAY_COUNT(stages)); + + static constexpr StageFn kStageFns[] = { +#define M(name) &Exec_##name, + SKCMS_ALL_OPS(M) +#undef M + }; + + for (ptrdiff_t index = 0; index < programSize; ++index) { + stages[index] = kStageFns[(int)program[index]]; + } + int i = 0; while (n >= N) { - exec_ops(program, contexts, src, dst, i); + exec_stages({stages}, contexts, src, dst, i); i += N; n -= N; } @@ -1463,7 +1478,7 @@ char tmp[4*4*N] = {0}; memcpy(tmp, (const char*)src + (size_t)i*src_bpp, (size_t)n*src_bpp); - exec_ops(program, contexts, tmp, tmp, 0); + exec_stages({stages}, contexts, tmp, tmp, 0); memcpy((char*)dst + (size_t)i*dst_bpp, tmp, (size_t)n*dst_bpp); } }
diff --git a/src/skcms_Transform.h b/src/skcms_Transform.h index 97413f4..51391a0 100644 --- a/src/skcms_Transform.h +++ b/src/skcms_Transform.h
@@ -17,7 +17,7 @@ /** All transform ops */ -#define SKCMS_LOAD_OPS(M) \ +#define SKCMS_ALL_OPS(M) \ M(load_a8) \ M(load_g8) \ M(load_4444) \ @@ -33,9 +33,7 @@ M(load_hhh) \ M(load_hhhh) \ M(load_fff) \ - M(load_ffff) - -#define SKCMS_WORK_OPS(M) \ + M(load_ffff) \ M(swap_rb) \ M(clamp) \ M(invert) \ @@ -84,31 +82,27 @@ M(table_a) \ \ M(clut_A2B) \ - M(clut_B2A) - -#define SKCMS_STORE_OPS(M) \ - M(store_a8) \ - M(store_g8) \ - M(store_4444) \ - M(store_565) \ - M(store_888) \ - M(store_8888) \ - M(store_1010102) \ - M(store_161616LE) \ - M(store_16161616LE) \ - M(store_161616BE) \ - M(store_16161616BE) \ - M(store_101010x_XR) \ - M(store_hhh) \ - M(store_hhhh) \ - M(store_fff) \ + M(clut_B2A) \ + M(store_a8) \ + M(store_g8) \ + M(store_4444) \ + M(store_565) \ + M(store_888) \ + M(store_8888) \ + M(store_1010102) \ + M(store_161616LE) \ + M(store_16161616LE) \ + M(store_161616BE) \ + M(store_16161616BE) \ + M(store_101010x_XR) \ + M(store_hhh) \ + M(store_hhhh) \ + M(store_fff) \ M(store_ffff) enum class Op : int { #define M(op) op, - SKCMS_LOAD_OPS(M) - SKCMS_WORK_OPS(M) - SKCMS_STORE_OPS(M) + SKCMS_ALL_OPS(M) #undef M };