From 892af1ec7bb0deb5f40557a0d4838df9dd74a0b6 Mon Sep 17 00:00:00 2001 From: Mike Klein Date: Mon, 8 May 2017 15:17:57 -0400 Subject: [PATCH] add a guide to contributing to SkJumper Change-Id: Icd40cf5eff3d2156a3ca00d7950059d5b77f48bf Reviewed-on: https://skia-review.googlesource.com/15890 Reviewed-by: Ben Wagner Commit-Queue: Mike Klein --- site/dev/contrib/jumper.md | 102 +++++++++++++++++++++++++++++++++++++ src/jumper/build_stages.py | 3 +- 2 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 site/dev/contrib/jumper.md diff --git a/site/dev/contrib/jumper.md b/site/dev/contrib/jumper.md new file mode 100644 index 0000000000..762e9b4faa --- /dev/null +++ b/site/dev/contrib/jumper.md @@ -0,0 +1,102 @@ +Contributing to SkJumper +======================== + +SkJumper is the execution engine of SkRasterPipeline, a system we've been using +to accelerate CPU-bound work inside Skia, most notably color-space conversions +and color-correct drawing. + +(This is where I'd put my link to design document if I had one...) + +SkJumper is more annoying to contribute to than most Skia code because of its +offline compilation step. `src/jumper/build_stages.py` compiles +`src/jumper/SkJumper_stages.cpp` several different ways and parses the object +files it generates into `src/jumper/SkJumper_generated.S` and +`src/jumper/SkJumper_generated_win.S`. This document is designed to guide you +through this process and ease some of that annoyance. + +One-time Setup +-------------- + +To run `build_stages.py` you need Clang 4.0 and objdump, and probably want +ccache. It's best that Clang is exactly the same version we typically use (as +of writing 4.0.0) and you'll need objdump to be compiled with support for +x86-64, ARMv7, and ARMv8. + +The easiest way to satisfy these contraints is to get your hands on a Mac and +install [Homebrew](https://brew.sh). Once you have `brew` installed, run this +to get the tools you need: + + $ brew install llvm binutils ccache + +Running `build_stages.py` +------------------------- + +With your tools installed, try a no-op run of `build_stages.py`: + + $ python src/jumper/build_stages.py path/to/clang-4.0 path/to/gobjdump path/to/ccache + $ git status + +When you run `git status` you should see a bunch of untracked `.o` files +sitting in skia/, and no changes to `src/jumper/SkJumper_generated*.S`. +That's good. Those object files are the intermediates we parse to produce +the assembly files. We just leave them around in case you want to look at +them yourself. If you don't like them, it's safe to just + + $ rm \*.o + +If `clang-4.0`, `gobjdump`, and `ccache` are on your path, `build_stages.py` +should find them without you needing to pass them on the command line. + +Make A Change +------------- + +Let's use the `from_srgb` stage as a little playground to make a real change. +Linearizing sRGB encoded bytes is slow, so let's pretend we've decided to trade +quality for speed, approximating the existing implementation with a simple square. + +Open up `SkJumper_stages.cpp` and find the `from_srgb` stage. It'll look like + + STAGE(from_srgb) { + ... + } + +Let's replace whatever's there with our fast approximation: + + STAGE(from_srgb) { + r *= r; + g *= g; + b *= b; + } + +When you save and re-run `build_stages.py`, you should now see changes to +`src/jumper/SkJumper_generated.S` and `src/jumper/SkJumper_generated_win.S`. +If you can't read assembly, no big deal. If you can, run `git diff`. You +should see the various `sk_from_srgb_*` functions get dramatically simpler, +something like three multiplies and a couple other bookkeeping instructions. + +It's not unusual for isolated changes in one stage to cause seemingly unrelated +changes in another. When adding or removing any code you'll usually see all +the comments in branch instructions change a little bit, but the actual +instruction on the left won't change. When adding or removing uses of +constants, you'll often see both the comment and instruction on the left change +for other loads of constants from memory, especially on x86-64. You'll also +see some code that looks like garbage change; those are the constants. If +any of this worries you, please do go running to someone who knows more for +help, but odds are everything is fine. + +At this point you can re-build Skia, run DM, compare images, etc. as normal. +Any time you change `SkJumper_stages.cpp`, you need to re-run `build_stages.py` +for those changes to take effect. Believe me, I'd bake this into our GN build +if I could figure out a way. + +Adding a new Stage +------------------ + +Adding a new stage is a lot like changing an existing stage. Edit +`SkJumper_stages.cpp`, run `build_stages.py`, build Skia, test, repeat until +correct. + +You'll just need to also edit `SkRasterPipeline.h` to add your new stage to the +macro listing all the stages. The stage name is the handle normal Skia code +uses to refer to the stage abstractly, and the wiring between +`SkRasterPipeline::foo` and `STAGE(foo) { ... }` should work automatically. diff --git a/src/jumper/build_stages.py b/src/jumper/build_stages.py index 26590bd314..301295fa68 100755 --- a/src/jumper/build_stages.py +++ b/src/jumper/build_stages.py @@ -11,8 +11,9 @@ import sys clang = sys.argv[1] if len(sys.argv) > 1 else 'clang-4.0' objdump = sys.argv[2] if len(sys.argv) > 2 else 'gobjdump' +ccache = sys.argv[3] if len(sys.argv) > 3 else 'ccache' -clang = ['ccache', clang, '-x', 'c++'] +clang = [ccache, clang, '-x', 'c++'] cflags = ['-std=c++11', '-Os', '-DJUMPER',