This is enough for another swath of tests.
Change-Id: Ida43fa2ee2ebd8e6086923fb9fafef8f646d0a93
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/274074
Commit-Queue: Herb Derby <herb@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Auto-Submit: Mike Klein <mtklein@google.com>
This means we can write a memset32 (load32 -> store32),
tested explicitly with the new unit test.
Slightly changes to the type protocol,
- load and splat now generate scalars or vectors
depending on how `scalar` is set
- store should no longer have to pay attention to `scalar`;
it's input values will already be the right size
Clean up some of the type declarations where we don't
actually need the subclass types, holding llvm::Type* instead.
This makes using ?: easier.
Change-Id: I2f98701ebdeead0513d355b2666b024794b90193
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/273781
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Change-Id: Ia1752196fd50ade2c3160dc401a36618433420d8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/270822
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
There are probably ways to make this more efficient by only optimizing
what's necessary (e.g. try JIT first, then interpreter only if it fails)
and some other performance improvements to make, but for now I want to
focus mostly on keeping things simple and correct.
The line between Builder::done() and Program::Program() is particularly
fuzzy and becoming fuzzier here, and I think that'll be something
that'll change eventually.
This makes SkVMTest debug dumps more portable, though perhaps less
useful. Might kill that feature soon now that SkVM is tested more
thoroughly in unit tests and GMs and bots and such.
Change-Id: Id9ce8daaf8570e5bea8b10f1a80b97f5b33d45dc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/269941
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Kind of brewing a big refactor here, to give me some room between
skvm::Builder and skvm::Program to do optimizations, bakend
specializations and analysis.
As a warmup, I'm trying to split up today's Builder::Instruction into
two forms, first just what the user requested in Builder (this stays
Builder::Instruction) then a new type representing any transformation or
analysis we've done to it (OptimizedInstruction).
Roughly six important optimizations happen in SkVM today, in this order:
1) constant folding
2) backend-specific instruction specialization
3) common sub-expression elimination
4) reordering + dead code elimination
5) loop invariant and lifetime analysis
6) register assignment
At head 1-5 all happen in Builder, and 2 is particularly
awkward to have there (e.g. mul_f32 -> mul_f32_imm).
6 happens in Program per-backend, and that seems healthy.
As of this CL, 1-3 happen in Builder, 4-5 now on this middle
OptimizedInstruction format, and 6 still in Program.
I'd like to get to the point where 1 stays in Builder, 2-5 all happen on
this middle IR, and 6 stays in Program. That ought to let me do things
like turn mul_f32 -> mul_f32_imm when it's good to and still benefit
from things like common sub-expression elimination and code reordering
happening after that trnasformation.
And then, I hope that's also a good spot to do more complicated
transformations, like lowering gather8 into gather32 plus some fix up
when targeting an x86 JIT but not anywhere else. Today's Builder is too
early to know whether we should do this or not, and in Program it's
actually kind of awkward to do this sort of thing while also doing
having to do register assignment. Some middle might be right.
Change-Id: I9c00268a084f07fbab88d05eb441f1957a0d7c67
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/269181
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This reverts commit d4e3b9e8bc.
Reason for revert: will reland with fixes
Original change's description:
> impl gather8/gather16 with gather32
>
> This is our quick path to JIT small gathers.
>
> The idea is roughly,
>
> const uint32_t* ptr32 = ptr8;
> uint32_t abcd = ptr32[ix/4];
> switch (ix & 3) {
> case 3: return (abcd >> 24) ;
> case 2: return (abcd >> 16) & 0xff;
> case 1: return (abcd >> 8) & 0xff;
> case 0: return (abcd ) & 0xff;
> }
>
> With the idea that if we may load a given byte,
> we should also be allowed to load the four byte
> aligned word that byte falls within.
>
> Change-Id: I7fb1085306050c918ccf505f1d2e1e87db3b8c9a
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/268381
> Reviewed-by: Herb Derby <herb@google.com>
> Commit-Queue: Mike Klein <mtklein@google.com>
TBR=mtklein@google.com,herb@google.com,reed@google.com
Change-Id: I48d800edc6517f37e04752c91616b666a5e0f384
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/268490
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This is our quick path to JIT small gathers.
The idea is roughly,
const uint32_t* ptr32 = ptr8;
uint32_t abcd = ptr32[ix/4];
switch (ix & 3) {
case 3: return (abcd >> 24) ;
case 2: return (abcd >> 16) & 0xff;
case 1: return (abcd >> 8) & 0xff;
case 0: return (abcd ) & 0xff;
}
With the idea that if we may load a given byte,
we should also be allowed to load the four byte
aligned word that byte falls within.
Change-Id: I7fb1085306050c918ccf505f1d2e1e87db3b8c9a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/268381
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Constant propagation means we can always notionally
unpremul and premul at the right points, and if alpha
was already opaque, they'll just drop away.
This has been true, but it's nice to put a test on it.
Change-Id: Iacd2002d9e1a10b73e800a452f377001d5ba3777
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/268336
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
- Add sqrt(), vsqrtps for x86.
- Hook into SkRadialGradient.
Change-Id: I66a4598e30fe16610c59a512f7d962323ee5134a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/267196
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This loads 32 bits instead of gathering 256 in the tail part of loops.
To make it work, add a vmovd with SIB addressing.
I also remembered that the mysterious 0b100 is actually a signal that
the instruction uses SIB addressing, and is usually denoted by `rsp`.
(SIB addressing may be something we'd want to generalize over like we
did recently with YmmOrLabel, but I'll leave that for Future Me.)
Slight rewording where "scratch" is mentioned to keep it focused on
scratch GP registers, not "tmp" ymm registers. Not a hugely important
distinction but helps when I'm grepping through code.
Change-Id: I39a6ab1a76ea0c103ae7d3ebc97a1b7d4b530e73
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/264376
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Some TODOs left over to make the scalar
tail case better... as is it issues a
256-bit gather for each 32-bit load!
I added a trimmed down variant of the existing
SkVM_gathers unit test to test just gather32,
covering this new JIT code.
Change-Id: Iabd2e6a61f0213b6d02d222b9f7aec2be000b70b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/264217
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This does the equivalent of dst = *(src + off),
which we use to find our gather base pointer in gather32.
Change-Id: I09ca7bfd404d7dce6de454ef1ed4eee78ab29932
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/264216
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
A complicated instruction to say the least!
A "fun" wrinkle is that all the ymm registers must be unique!
(And the mask register is cleared by the instruction...)
Still kind of TODO is what that 0b100 r/m in the mod_rm() means. Every
variant of the instruction I've assembled seems to have it set to 0b100
(e.g. 0x0c or 0x04) but I'd feel better if I knew what it meant.
Change-Id: Ia4ff5f8175bff545e2d10bb2d1b14f49073445a3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/264116
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
- Add YmmOrLabel struct to represent the concept that many
x86 instructions can take a final argument as either a
register or memory address, and that they all handle them
the same way.
- Convert existing overloads like vmulps() to use YmmOrLabel.
- upgrade some other instructions to take YmmOrLabel
- use them to implement today's new _imm ops
This feels like a good spot for implicit constructors, no?
Change-Id: I435028acc3fbfcc16f634cfccc98fe38bbce9d19
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/263207
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
- Remove extract... it's not going to have any special impl.
I've left it on skvm::Builder as an inline compound method.
- Add no-op shift short circuits.
- Add immediate ops for bit_{and,or,xor,clear}.
This comes from me noticing that the masks for extract today are always
immediates, and then when I started converting it to be (I32, int shift,
int mask), I realized it might be even better to break it up into its
component pieces. There's no backend that can do extract any better
than shift-then-mask, so might as well leave it that way so we can
dedup, reorder, and specialize those micro ops.
Will follow up soon to get this all JITing again,
and these can-we-JIT test changes will be reverted.
Change-Id: I0835bcd825e417104ccc7efc79e9a0f2f4897841
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/263217
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Basic support for clamp/clamp/nearest/RGBA-8888/premul images.
A couple little changes to make this work:
- add pixel-center offset to shader (x,y)
- change the signature of gather??() calls to work
more naturally with how we let effects build uniforms.
Instead of gathering directly from one of the program
arguments, load the gather base pointer off another
uniforms pointer, just like any other uniform.
- remove the default argument to uniform??() so that
they parallel the new gather??() calls more closely.
There was only one place that was using the default
and I think it's clearer as an explicit 0 offset.
- centralize some more helpers onto skvm::Builder so
we can use the in both SkVMBlitter and SkImageShader.
Some diffs:
- very, very small color diffs probably due to slightly
different math converting between byte and float or blending;
- small sampling coordinate diffs where skvm + SkRP agree,
and the legacy shaders disagree. That's fine by me.
Change-Id: I72634e7fed4f13e6cb41b8067104760f392ea3bf
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/262368
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Reed <reed@google.com>
These are really designed around x86, so forcing them
on ARM where our existing non-immediate ops work better
is kind of silly.
Change-Id: I6b66ed0b0a71b335becdcb1d67dec471620542b0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/254440
Reviewed-by: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This all comes together as
uminv tmp, condition
fmov gp, tmp
cbnz gp, all_true
brk 0
all_true:
...
The key idea is uminv(vec) will return 0 if any of the inputs are 0,
and non-zero if all of the inputs are non-zero, namely 0xffffffff.
fmov moves that minimum from a vector register to a general purpose
register where we can test it with cbnz, compare and branch if non-zero.
This jumps over the `brk 0` debug trap when all inputs are true.
Change-Id: If5deb77a77f52221d0649e537179743c45eb9cc5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/254479
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Lots of x86 instructions can take their right hand side argument from
memory directly rather than a register. We can use this to avoid the
need to allocate a register for many constants.
The strategy in this CL is one of several I've been stewing over, the
simplest of those strategies I think. There are some trade offs
particularly on ARM; this naive ARM implementation means we'll load&op
every time, even though the load part of the operation can logically be
hoisted. From here on I'm going to just briefly enumerate a few other
approaches that allow the optimization on x86 and still allow the
immediate splats to hoist on ARM.
1) don't do it on ARM
A very simple approach is to simply not perform this optimization on
ARM. ARM has more vector registers than x86, and so register pressure
is lower there. We're going to end up with splatted constants in
registers anyway, so maybe just let that happen the normal way instead
of some roundabout complicated hack like I'll talk about in 2). The
only downside in my mind is that this approach would make high-level
program descriptions platform dependent, which isn't so bad, but it's
been nice to be able to compare and diff debug dumps.
2) split Op::splat up
The next less-simple approach to this problem could fix this by
splitting splats into two Ops internally, one inner Op::immediate that
guantees at least the constant is in memory and is compatible with
immediate-aware Ops like mul_f32_imm, and an outer Op::constant that
depends on that Op::immediate and further guarantees that constant has
been broadcast into a register to be compatible with non-immediate-aware
ops like div_f32. When building a program, immediate-aware ops would
peek for Op::constants as they do today for Op::splats, but instead of
embedding the immediate themselves, they'd replace their dependency with
the inner Op::immediate.
On x86 these new Ops would work just as advertised, with Op::immediate a
runtime no-op, Op::constant the usual vbroadcastss. On ARM
Op::immediate needs to go all the way and splat out a register to make
the constant compatible with immediate-aware ops, and the Op::constant
becomes a noop now instead. All this comes together to let the
Op::immediate splat hoist up out of the loop while still feeding
Op::mul_f32_imm and co. It's a rather complicated approach to solving
this issue, but I might want to explore it just to see how bad it is.
3) do it inside the x86 JIT
The conceptually best approach is to find a way to do this peepholing
only inside the JIT only on x86, avoiding the need for new
Op::mul_f32_imm and co. ARM and the interpreter don't benefit from this
peephole, so the x86 JIT is the logical owner of this optimization.
Finding a clean way to do this without too much disruption is the least
baked idea I've got here, though I think the most desirable long-term.
Cq-Include-Trybots: skia.primary:Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-SK_USE_SKVM_BLITTER,Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Release-All-SK_USE_SKVM_BLITTER
Change-Id: Ie9c6336ed08b6fbeb89acf920a48a319f74f3643
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/254217
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
The logic implemented here is roughly
assert_true(v):
if any ~v {
int3()
}
in assembly as
```
vptest v, constant 0xffffffff mask
jc ok
int3
ok:
```
jc branches if (~v & mask) are all zero, with mask set fully, that's
branch if ~v are all zero, which is to say, v are all ~0, true. So we
jump over the int3 breakpoint if v are all true.
Cq-Include-Trybots: skia.primary:Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-SK_USE_SKVM_BLITTER
Change-Id: Ie0fc1da15b1a0dba00c66af610ccde18f5985f8a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/253897
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Auto-Submit: Mike Klein <mtklein@google.com>
Will use these to implement assert_true on x86.
Change-Id: I9d2595a35518b6971dd8e418b583febd3960c7f6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/253896
Commit-Queue: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Reed <reed@google.com>
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Reed <reed@google.com>
This is an assert that is active in debug mode. For the moment it only
works in the interpreter, but I plan to follow up with JIT code too.
assert_true() is a data sink like a store() as far as lifetime goes,
though we take care to allow it to be hoisted if its inputs are. An
assert_true's existence will keep all its inputs alive, and in release
builds where we skip the instruction, those inputs will all drop away
automatically.
Tested locally by forcing the interpreter. It shouldn't be long before
I have at least x86 JIT asserts working too.
Change-Id: I7aba40d040436a57a6b930790f7b8962bafb1a8c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/253756
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This plumbs through round but doesn't use it. I want that change to be
its own CL. It's nice to have assembler support and the name changes
even if I revert using round.
Change-Id: I6d67ec5c63546069eb7cc1c91599b599bafcda66
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/253724
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
No diffs.
Change-Id: Ia0b35c2787e27d74763f21b81072affa6caf1e5a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/253720
Commit-Queue: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Reed <reed@google.com>
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Reed <reed@google.com>
This is a reland of 12cea8d6c4
Now implementing float comparisons on ARM also.
Only vaguely tricky thing is that x!=y is ~(x==y).
Original change's description:
> hook up float comparisons to x86 JIT
>
> This gets the draws in gm/skvm.cpp all JITing again,
> and in one of the unit tests.
>
> (Everything draws the same of course.)
>
> Change-Id: Iada28690d9df78f9d444ee3765e21beb29239672
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/253166
> Auto-Submit: Mike Klein <mtklein@google.com>
> Reviewed-by: Mike Klein <mtklein@google.com>
> Commit-Queue: Mike Klein <mtklein@google.com>
Cq-Include-Trybots: skia.primary:Test-Android-Clang-NVIDIA_Shield-CPU-TegraX1-arm64-Debug-All-Android
Change-Id: I771b8a327a958db8a0d509d55863ade935a00035
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/253401
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
JIT code isn't MSAN-instrumented, so we won't see when it uses
uninitialized memory, and we'll not see the writes it makes as properly
initializing memory. Instead force the interpreter, which should let
MSAN see everything our programs do properly.
This refactors so that SkVM.cpp is the only code to look at whether
SKVM_JIT is defined, and undefines it when built with MSAN. Added
a simple regression test too.
Cq-Include-Trybots: skia.primary:Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-MSAN
Change-Id: Ic7cca2621f84dfba7174127738744d6c68f85f2e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/253410
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This reverts commit 12cea8d6c4.
Reason for revert: unit tests failing on ARM... will try again once I have float comparisons implemented for ARM too.
Original change's description:
> hook up float comparisons to x86 JIT
>
> This gets the draws in gm/skvm.cpp all JITing again,
> and in one of the unit tests.
>
> (Everything draws the same of course.)
>
> Change-Id: Iada28690d9df78f9d444ee3765e21beb29239672
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/253166
> Auto-Submit: Mike Klein <mtklein@google.com>
> Reviewed-by: Mike Klein <mtklein@google.com>
> Commit-Queue: Mike Klein <mtklein@google.com>
TBR=mtklein@google.com,reed@google.com
Change-Id: Ie07e580b4998199338217a27d4fad34c679ffc23
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/253399
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This gets the draws in gm/skvm.cpp all JITing again,
and in one of the unit tests.
(Everything draws the same of course.)
Change-Id: Iada28690d9df78f9d444ee3765e21beb29239672
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/253166
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This lets shaders that use 'x' JIT on x86.
I started with paddd and {0,-1,-2,-3,...}, which worked fine but on
second thought seemed a bit odd. I've switched to psubd and
{0,1,2,3,...} but I've left in support for paddd with a memory arg.
gm/skvm.cpp now JITs fully again and continues to draw the same as
the interpreter did.
Simplify embedded data alignment a little... memory operands don't
need full register alignment in AVX like they used to in SSE. So
just align everything to the vector element size like we do on ARM,
and reorder [splats,bytes_masks,iota] to match the order we declare
and handle them in the code above.
Add unit tests for vpaddd + vpsubd.
Cq-Include-Trybots: skia.primary:Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-SK_USE_SKVM_BLITTER
Change-Id: I6b8d060450cca7f437a1d2a597a8a0e0e8d51b33
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/252797
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Reed <reed@google.com>
Rewrite program instructions so that each value becomes available as
late as possible, just before it's used by another instruction. This
reorders blocks of instructions to reduce them number of temporary
registers in flight.
Take this example of the sort of program that we naturally write,
noting the registers needed as we progress down the right:
src = load32 ... (1)
sr = extract src ... (2)
sg = extract src ... (3)
sb = extract src ... (4)
sa = extract src ... (4, src dies)
dst = load32 ... (5)
dr = extract dst ... (6)
dg = extract dst ... (7)
db = extract dst ... (8)
da = extract dst ... (8, dst dies)
r = add sr dr (7, sr and dr die)
g = add sg dg (6, sg and dg die)
b = add sb db (5, sb and db die)
a = add sa da (4, sa and da die)
rg = pack r g ... (3, r and g die)
ba = pack b a ... (2, b and a die)
rgba = pack rg ba ... (1, rg and ba die)
store32 rgba ... (0, rgba dies)
That original ordering of the code needs 8 registers (perhaps with a
temporary 9th, but we'll ignore that here). This CL will rewrite the
program to something more like this by recursively issuing inputs only
once needed:
src = load32 ... (1)
sr = extract src ... (2)
dst = load32 ... (3)
dr = extract dst ... (4)
r = add sr dr (3, sr and dr die)
sg = extract src ... (4)
dg = extract dst ... (5)
g = add sg dg (4, sg and dg die)
rg = pack r g (3, r and g die)
sb = extract src ... (4)
db = extract dst ... (5)
b = add sb db (4, sb and db die)
sa = extract src ... (4, src dies)
da = extract dst ... (4, dst dies)
a = add sa da (3, sa and da die)
ba = pack b a (2, b and a die)
rgba = pack rg ba ... (1, rg and ba die)
store32 rgba ... (0)
That trims 3 registers off the example, just by reordering!
I've added the real version of this example to SkVMTest.cpp.
(Its 6th register comes from holding the 0xff byte mask used
by extract, in case you're curious).
I'll admit it's not exactly easy to work out how this reordering works
without a pen and paper or trial and error. I've tried to make the
implementation preserve the original program's order as much as makes
sense (i.e. when order is an otherwise arbitrary choice) to keep it
somewhat sane to follow.
This reordering naturally skips dead code, so pour one out for ☠️ .
We lose our cute dead code emoji marker, but on the other hand all code
downstream of Builder::done() can assume every instruction is live.
Change-Id: Iceffcd10fd7465eae51a39ef8eec7a7189766ba2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/249999
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
These implement select, eq_i32, lt_i32, and gt_i32 on ARMv8.
Change-Id: Ic36dda1cc425ca91700f9b120594e420ea0f560a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/248970
Auto-Submit: Mike Klein <mtklein@google.com>
Commit-Queue: Herb Derby <herb@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Most hoisted values are used in the loop body (and that's really the
whole point of hoisting) but some are just temporaries to help produce
other hoisted values. This used_in_loop bit helps us distinguish the
two, and lets us recycle registers holding temporary hoisted values not
used in the loop.
The can-we-recycle logic now becomes:
- is this a real value?
- is it time for it to die?
- is it either not hoisted or a hoisted temporary?
The set-death-to-infinity approach for hoisted values is now gone. That
worked great for hoisted values used inside the loop, but was too
conservative for hoisted temporaries. This lifetime extension was
preventing us from recycling those registers, pinning enough registers
that we run out and fail to JIT.
Small amounts of refactoring to make this clearer:
- move the Instruction hash function definition near its operator==
- rename the two "hoist" variables to "can_hoist" for Instructions
and "try_hoisting" for the JIT approach
- add ↟ to mark hoisted temporaries, _really_ hoisted values.
There's some redundancy here between tracking the can_hoist bit, the
used_in_loop bit, and lifetime tracking. I think it should be true, for
instance, that !can_hoist && !used_in_loop implies an instruction is
dead code. I plan to continue refactoring lifetime analysis (in
particular reordering instructions to decrease register pressure) so
hopefully by the time I'm done that metadata will shake out a little
crisper.
Change-Id: I6460ca96d1cbec0315bed3c9a0774cd88ab5be26
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/248986
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
This is a reland of 52435503e9
with better checks for when we should expect JIT and not.
Original change's description:
> mark which SkVM tests should JIT or not
>
> Most of these tests converted over to test_interpreter_only()
> are failing to JIT because of unimplemented instructions. No
> bug there, just TODOs.
>
> But SkVM_hoist _should_ be JITting. A while back I landed a CL
> that messed with value lifetimes that prevents it from JITting.
> Will be using this as a regression test to fix that bug.
>
> Change-Id: Id2034f6548a45ed9aeb9ae3cbb24d389cad7dc60
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/248980
> Commit-Queue: Mike Klein <mtklein@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Auto-Submit: Mike Klein <mtklein@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: Herb Derby <herb@google.com>
Cq-Include-Trybots: skia.primary:Test-Android-Clang-NVIDIA_Shield-CPU-TegraX1-arm64-Release-All-Android,Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Release-All-SK_CPU_LIMIT_SSE2,Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Release-All-SK_CPU_LIMIT_SSE41,Test-Mac10.13-Clang-VMware7.1-CPU-AVX-x86_64-Debug-All-NativeFonts,Test-Mac10.14-Clang-VMware7.1-CPU-AVX-x86_64-Debug-All-NativeFonts
Change-Id: Id7bde7e879649e435fa424a9c9d6c51a31afd5e9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/248990
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This reverts commit 52435503e9.
Reason for revert: lots of bots can't JIT, duh...
Original change's description:
> mark which SkVM tests should JIT or not
>
> Most of these tests converted over to test_interpreter_only()
> are failing to JIT because of unimplemented instructions. No
> bug there, just TODOs.
>
> But SkVM_hoist _should_ be JITting. A while back I landed a CL
> that messed with value lifetimes that prevents it from JITting.
> Will be using this as a regression test to fix that bug.
>
> Change-Id: Id2034f6548a45ed9aeb9ae3cbb24d389cad7dc60
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/248980
> Commit-Queue: Mike Klein <mtklein@google.com>
> Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
> Auto-Submit: Mike Klein <mtklein@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
> Reviewed-by: Herb Derby <herb@google.com>
TBR=mtklein@google.com,herb@google.com,ethannicholas@google.com
Change-Id: Ieea4b06f0d32249e3da56c6810d3c45c2abf2689
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/248989
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Most of these tests converted over to test_interpreter_only()
are failing to JIT because of unimplemented instructions. No
bug there, just TODOs.
But SkVM_hoist _should_ be JITting. A while back I landed a CL
that messed with value lifetimes that prevents it from JITting.
Will be using this as a regression test to fix that bug.
Change-Id: Id2034f6548a45ed9aeb9ae3cbb24d389cad7dc60
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/248980
Commit-Queue: Mike Klein <mtklein@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Herb Derby <herb@google.com>
I happened to have this on when profiling skottie_tool and got curious
why I was seeing the interpreter run and not JIT code. Mostly this
moves the code in bulk out of SkVMTest.cpp to SkVM.cpp so that code in
SkVM.cpp can call dump() on itself.
Also this CL has the skvm::Program hang onto the original value-based
builder program (in addition to its own interpreter program and JIT
program if we can). This is entirely so that when JIT bails out I
can have it dump out both the builder and interpreter programs for
more debugging aid.
I'm still going to need more debug tools somewhere to figure out
what the program that needs 17 registers is, and what to do about
it.
Finally, remove skvmtool. It's annoying to maintain its build
rules, and I don't use it much if ever anymore.
Change-Id: I995d15d04bda79ddfc4d68bda8aaa3b5b9261f08
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/242520
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This makes the register recycling checks a bit more
precise. At head we never recycle a register that's
holding a hoisted value, which is overly conservative.
We really should never recycle a register that's still
needed. By extending the lifetime of any hoisted value
that's used in the loop, we prevent that, while still
allowing hoisted values that are only used in hoisted
computation to be reused.
This takes just a small tweak in the JIT code (removing
the !hoisted({x,y,z}) checks), and a somewhat larger
refactoring in the interpreter, making both hoisted and
non-hoisted code go through the same recycling register
assignment flow.
There's one diff in the existing cases where we now
reuse a hoisted register, and I've added a second test
just to make sure it's covered explicitly.
Change-Id: I25b37ab1f1fea3042d7fd167529abc8fed1dddff
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/233239
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Add vpblendvb, vpcmpeqd, and vpcmpgtd, to implement select and eq/lt/gt.
I want to think just a touch bit more about neq, lte, and gte.
This is enough to JIT everything SkVMBlitter creates today.
There are 24 possible argument orders to vpblendvb,
so I'm sure I've got them wrong somehow, even with the new test.
Change-Id: I357664b866d8258a2b5438d520f47542ad581c50
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/232060
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Nothing particularly tricky. Very much like store8,
but with one fewer shuffle.
This lets some of the 565 blitters JIT!
Change-Id: I853905bda30a0cda89f3fcb5fef1dfe62725063b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/232059
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Very similar to load8.
The only interesting thing is how different vpinsrw is from vpinsrb (and
vpinsrd and vpinsrq)... different map of instructions entirely.
Change-Id: Ia413b83604dd2d277d59495c5f693f505c35be9f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/232058
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Add vbroadcastss(Ymm, Xmm) and expand movzbl() to support an offset.
The sequence
movzbl (load a byte, zero-extend to 4 byte)
vmovd (move that 4 byte value to an xmm)
vbroadcastss (broadcast low 4 bytes of xmm to all lanes of ymm)
implements uniform8.
Change-Id: I1d3125920d19dcb3cad5980495310bc95b9dffee
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/232057
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
vbroadcastss with a register argument and immediate offset implements
uniform32. And this turns on the JIT for some new SkVMBlitter paths
that pass more arguments that I'd previously wired up, so add a few
more.
Change-Id: I66db1286dcdb2c4a4ba7c43f2dc2cd13564d4d34
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/232056
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Just because a program doesn't read from or
write to memory doesn't mean it's pointless.
Oh wait, yes it does. It shouldn't crash though.
Change-Id: I6a9c26c065831f9598afccce6e0a34a178cbd925
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/230839
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Fills in a little TODO.
Also add assembler unit tests for similar float->int and int->float.
Tested by SkVM_mad, SkVM_madder.
Change-Id: I5334029927fdecb0ff7f5a3b081cf2ce7b23995c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/230838
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Backfill tests for some uncovered operations.
Mostly functional tests, with some targeted at particular
corners of code generation, and a few assembler unit tests.
This doesn't get up to 100% line coverage, but I think it
covers all the non-failure cases.
Change-Id: Ib701df0c505aa41e3b2fe3cd429447acb294b752
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/230837
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Change-Id: I58b52d3e1d05d0834be30e00d991636e227cbf0b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/230836
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Add missing comparison and selection ops, bit casts, 16-bit memory
operations, gathers, uniform loads, and fill in math holes where
reasonable. Update some names to be a bit more regular.
I think all instructions are implemented in the interpreter,
and many tested. More testing and JITs to follow.
Change-Id: I8cf377e8b72a86ac950e020892ce82b39e9d7277
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/229893
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This first tries to JIT while hoisting all constants,
and if that fails, tries again hoisting no constants.
I figure this is one of those 80/20 deals for how to
handle constant hoisting and register pressure. This
probably mostly moots doing anything fancy like using
memory operands with AVX or lane operands with NEON.
This _doesn't_ moot hoisting the NEON tbl arguments,
which is not yet done here, but probably my next CL.
Change-Id: Id09d5cdddcdb45207bdfc914a5a3128a481a26f3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/229058
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Even if a JIT ultimately doesn't end up hoisting any values, it's going
to want this information while it decides. Writing it in one place also
ensures we only get it wrong in one place...
I'm no_ extending the lifetime of hoisted instructions here in Builder.
That's something to leave to the backend so they have the flexibility of
which of these values to hoist, if any. If they don't hoist, they'll
need to know when the value dies.
Moving this information back here lets the test expectation goldens
reflect the hoist bit again too. Kind of nice.
Change-Id: Ib165ca898a97c1d822cb28fe24f15bae4d570a17
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/229024
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
We can always move data around so that an FMA is possible using no more
registers than we would otherwise, and on x86, evne using no more
instructions.
The basic idea here is that if we can't reuse one of the inputs to
destructively host the FMA instruction, the next best thing is to copy
one of the arguments into tmp() and accumulate the FMA there.
Once the FMA has happened, we just need to copy that result to dst().
We can of course skip that copy if dst() == tmp(). On x86 we never need
that copy; dst() and tmp() are picked using the same logic except that
dst may alias one of its inputs, and we only fall into this case after
we've already found it doesn't. So we can just assert dst() == tmp()
rather than check it like we do on ARM.
It's subtle, but I think sound.
I'm using logical-or to copy registers around. This is a little lazy,
but maybe not as lazy as it looks: on ARM that is _the_ way to copy
registers. There's a vmovdqa instruction I could use on x86, TBD.
All paths through this new code were being exercised on ARM, but we
didn't have anything hitting the tmp case on x86, so I've added a new
unit test that hits the corner cases of both implementations.
Change-Id: I5422414fc50c64d491b4933b4b580b784596f291
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228630
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
We have pack(x,y,imm) = x | (y<<imm) assuming (x & (y<<imm)) == 0.
If we can destroy x, sli (shift-left-insert) lets us implement that
as x |= y << imm. This happens quite often, so you'll see sequences
of pack that used to look like this
shl v4.4s, v2.4s, #8
orr v1.16b, v4.16b, v1.16b
shl v2.4s, v0.4s, #8
orr v0.16b, v2.16b, v3.16b
shl v2.4s, v0.4s, #16
orr v0.16b, v2.16b, v1.16b
now look like this
sli v1.4s, v2.4s, #8
sli v3.4s, v0.4s, #8
sli v1.4s, v3.4s, #16
We can do this thanks to the new simultaneous register assignment
and instruction selection I added. We used to never hit this case.
Change-Id: I75fa3defc1afd38779b3993887ca302a0885c5b1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228611
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Trying to keep most of the structural parts shared between x86_64 and
aarch64. Not sure if this will stay factored like this long-term, but
the last version felt like there was a bit too much redundancy, and I
don't want to write things like register management more often than have
to.
Change-Id: Ieeb21f433715a730c41c85d657c5b33fa4702696
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228608
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
- get rid of variadic Assembler::byte()... not used very often
- rename Assembler::byte(ptr,n) to bytes()
- align with 0 bytes, get rid of nop()
Change-Id: I7564d3bad00e3f0d1c7a80153c445966914fccf0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228601
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This is a reland of 558b639225
PS2... oh, right, not everything supports AVX2.
Original change's description:
> more JIT refactoring
>
> This re-enables AVX2 JIT with simultaneous register assignment and
> instruction selection. You can see it working in a very basic way in
> how we choose instructions and registers for Op::mad_f32.
>
> Constants are still broadcast, here inside the loop instead of hoisted.
> I think it'll probably end up best to use constants directly from memory
> (as in vpshufb's masks), falling back to these in-loop broadcasts when
> that can't work.
>
> Change-Id: If17d51b9960f08da3612e51ac04424e996bf83d4
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228366
> Commit-Queue: Mike Klein <mtklein@google.com>
> Reviewed-by: Mike Klein <mtklein@google.com>
Cq-Include-Trybots: skia.primary:Test-Mac10.13-Clang-VMware7.1-CPU-AVX-x86_64-Debug-All-NativeFonts
Change-Id: I6f99d275040abe6210a980fc544f7f22c3b85727
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228476
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This reverts commit 558b639225.
Reason for revert: broke some perf bots
Original change's description:
> more JIT refactoring
>
> This re-enables AVX2 JIT with simultaneous register assignment and
> instruction selection. You can see it working in a very basic way in
> how we choose instructions and registers for Op::mad_f32.
>
> Constants are still broadcast, here inside the loop instead of hoisted.
> I think it'll probably end up best to use constants directly from memory
> (as in vpshufb's masks), falling back to these in-loop broadcasts when
> that can't work.
>
> Change-Id: If17d51b9960f08da3612e51ac04424e996bf83d4
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228366
> Commit-Queue: Mike Klein <mtklein@google.com>
> Reviewed-by: Mike Klein <mtklein@google.com>
TBR=mtklein@google.com
Change-Id: Id6cd5acd873499bb394009489d77e7636ecbc9c6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228462
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This re-enables AVX2 JIT with simultaneous register assignment and
instruction selection. You can see it working in a very basic way in
how we choose instructions and registers for Op::mad_f32.
Constants are still broadcast, here inside the loop instead of hoisted.
I think it'll probably end up best to use constants directly from memory
(as in vpshufb's masks), falling back to these in-loop broadcasts when
that can't work.
Change-Id: If17d51b9960f08da3612e51ac04424e996bf83d4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228366
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
I find myself passing around parallel vectors of Builder::Instructions
and deaths so often that it just makes more sense practically to store
them together. It's a little awkward that the values are only useful
after calling done(), but I can live with that.
Get a little more careful about mutation, passing Builder::Instructions
by const&. Instead of extending lifetimes of live hoisted
instructions, just check for them in maybe_recycle_register() instead.
Change-Id: I1cb9e25c1a7c46a250c2271334821be8535353bf
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228367
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
I'm slowly refactoring my way to where hoisting and register assignment
are done in backend-specific ways, but this liveness analysis is always
going to be useful for each backend.
Use deaths() to restore friendly ☠️ dead code markers in test dumps.
Change-Id: I3ab94665bbbbf0788b0b27e00d644eba927dff47
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/228113
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Add Program::dropJIT() to allow us to proactively drop
any JIT code forcing fallback on the interpreter,
and use it to test both on JIT-supported platforms.
Other platforms will just test the interpreter twice.
Change-Id: I607d00ef3c648e66a0b3a1374b11aa82dbfff70c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/227424
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Arg strides are the reason JIT happens lazily in Program::eval() today
instead of proactively in Builder::done() or Program's constructor. It
also just really doesn't make sense to delay this information... it's
not like you can change it up sanely between calls to eval().
The argument index now comes implicitly from the order of calling arg().
This may seem logically independent, but it prevents a weird situation
where you could use the same argument index twice with different
strides... not sure what that would mean.
Change-Id: I0f5d46e94a1ca112a72675c5492f17c0dd825ce0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/227390
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Basically the same deal as aarch64:
- a bunch of instructions to rewrite control
flow to be two loops, body and tail
- a bunch of instructions to support scalar
loads and stores in the tail
We can now remove the JIT::mask field.
I've removed the SkUNREACHABLE I'd put in for the ARM code... as
written the interpreter is still reachable by the loser if two threads
race to JIT the program. Medium term I plan to move JIT compilation to
a more proactive time, eliminating the need for the lock and letting the
interpreter become truly unreachable.
I had a little bit of a false start with what instructions to use for
scalar load8 and store8, first starting with instructions that loaded
via GP registers, then remembering vpinsrb and vpextrb can take a memory
argument, loading into xmm directly. I've left the first instructions I
used in the file, still implemented but only used from the unit tests.
They're pretty common and will probably be useful some day.
Change-Id: I471b13026af4b1c6e861a53159f9df5f0285447c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/227178
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
I had been setting the REX R bit to select high registers,
but you actually set the B bit. Don't know how I got that
wrong before... the leading byte should be 49 not 4c.
$ cat test.s
foo:
addq $7, %r8
$ clang -c test.s && objdump -d test.o
0000000000000000 <foo>:
0: 49 83 c0 07 add $0x7,%r8
Change-Id: I039e1c4f4ea20523a1e2cc9bcf5f6d9321a6223b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/227177
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This turned out to be quite an easy transformation
with yesterday's work already done. No codegen changes.
Change-Id: Ife19ab7731514c54cfed963a6d2e9b1ec2246997
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/227137
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Support forward references in Label.
In addition to tracking the current Label offset (used for
backward references essentially just the same as before this CL)
we also store a list of instructions that refer to each Label.
When a Label moves, each instruction gets a new displacement.
To make this a little easier, remove the 8-bit jump form on x86...
this way all x86 displacements are 32-bit and and all ARM 19-bit.
For now only cbz() supports this, just to start somewhere.
More to do but it's worth an early design review.
Change-Id: I23d2bcd7742965ab694ae4828f53409cb9fc807f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/226937
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
This adds a bunch of instructions we'll need to handle the N < 4 tail
within the JIT code on ARM.
- ldrb/strb are 1-byte load and stores
- sub subtracts without setting flags
- cmp just sets flags (actually just subs with an xzr destination)
- add b and b.lt, just like b.ne
- cbz and cbnz... we only need cbz but I accidentally did cbnz first
Once I add support for forward jumps, we'll be able to use these
instructions to restructure the loop to
entry:
hoisted setup
loop:
if N < 4, jump tail (cmp N,#4; b.lt tail)
... handle 4 values ...
jump loop (b loop)
tail:
if N == 0, jump end (cbz N, end)
... handle 1 value ...
jump tail (b tail)
end:
ret
Change-Id: I62d2d190f670f758197a25d99dfde13362189993
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/226828
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Noticed we were only dumping the final register
programs for the integer code. Might as well also
track the value programs.
Change-Id: I417c5c655b632691557bbbb136dcbd3f3167af9a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/225324
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
This is test-only code only used by SkVMTest.cpp,
so it can live there. This cuts the dependency
of SkVM on SkStream and co.
Change-Id: I7695e527b2d16e4485f8c5f4cd39bb8300e9221d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/225321
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Some small refactoring to common up redundant opcode building.
Oddly, I think I've got better codegen than what Clang would do here.
Clang doesn't generate uxtl-based code to unpack 8-bit to 32-bit,
instead preferring to load each byte one at a time and insert them one
at a time.
Me:
ldr s0, [x0]
uxtl v0.8h, v0.8b
uxtl v0.4s, v0.8h
Clang:
ldrb w8, [x0]
ldrb w9, [x0, #1]
ldrb w10, [x0, #2]
ldrb w11, [x0, #3]
fmov s0, w8
mov v0.s[1], w9
mov v0.s[2], w10
mov v0.s[3], w11
Change-Id: I0fdf5c6cdcde6a4eb9290936284fd3ffcb2159f6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/224821
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
So far this is just as easy as I had hoped.
Change-Id: I5f69a900b32d9bf70156b55e334233d7376b820f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/223340
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Instead of allocating into a std::vector, we do one quick first pass to
measure how much memory we need to allocate, mmap enough pages for that,
then another real writing pass.
This cuts a microsecond or so off the profile. There's another
microsecond left to cut if we could eliminate that first measuring pass,
but I'm no longer sure it's easy to come up with a good upper limit on
the program size now that I'm thinking about the data part of the
program as well.
vpshufb is our current max instruction at 9 bytes of code, but that also
implies another 32 bytes of control data. I'm not sure I feel very
clever allocating 41 * |instructions| bytes to be conservatively safe...
it seems like ridiculous overkill.
Ultimately I found it easier to just measure twice, cut once.
Change-Id: I16ccdafbc789711837b41b3d5a557808798eb1b4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/223305
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
I was just reading the ARM docs and realized that their BIC ("BIt
Clear") is the same as SSE's ANDN ("AND Not") instruction. It's kind of
a neat little tool to have laying around... comes up more than you'd
think, and it's sometimes the clearest way to express what you're doing,
as in the changed program here where the comment is "mask away the low
bits". That's a bit_clear with a mask for what you want to clear away!
And the real reason to write this up is that I want to have a CL to
point to that shows how to add an instruction top to bottom.
Change-Id: I99690ed9c1009427b3986955e7ae6264de4d215c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/223120
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Reviewed-by: Mike Reed <reed@google.com>
For now, disable the vpmovusdb AVX-512 instruction, using the compound
AVX2 fallback instead. I need to learn how to encode EVEX prefixes
before we can use that, and it's not very important.
That's everything! We're fully in control now, and should be able to
run this on any x86-64 Linux or Mac. And we can relax some of the
defined(SKVM_JIT) guards so that, e.g., we can unit test Assembler even
on all platforms.
Stifle some warnings about ~bool by ~(int)bool.
Would like to enable when is_mac too but can't seem to get past
(bogus?) thread annotation on the bots. My local Mac is fine. :/
Change-Id: If00bdd97ebd9684ed109933e2fa70c5e6f6ea339
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/222631
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
This shows off a little how easy backwards-only labels are.
The rip == rbp + Mod::Indirect convention isn't something
you'd be able to guess without just looking at the docs.
I'm not actually sure if you can only use rbp or also r13,
but LLVM seems to always do the equivalent of rbp... might
just be that high bit in VEX is ignored: they're registers
5 and 13, 8 apart, only distinguished by that bit.
Convenienly RIP addressing is always 32-bit, so there's
no benefit to spending time checking whether the offset
fits in a byte, though most of our offsets would.
Change-Id: I01b7fb1500667e1bf98490d5144459f92e1b375d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/222857
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
By putting data first in descending alignment then code, we never need
any alignment padding.
This also makes all jumps and ip-relative data loads backward, so
they're really easy to assemble. No need for any sort of deferred
where-does-this-label-mean logic; the label can just be a simple byte
offset established before you need to use it.
Nothing new switched off of Xbyak in this CL, but the rearrangement
makes the rest a lot easier.
The one downside I've found so far is that the disassembly of the
first instruction can get confused into data or other instructions,
e.g.
63: 01 ff add %edi,%edi
65: 00 ff add %bh,%bh
67: 00 00 add %al,(%rax)
69: ff 00 incl (%rax)
6b: ff c4 inc %esp
6d: e2 7d loop ec <skvm-jit-884702985+0xac>
6f: 18 05 eb ff ff ff sbb %al,-0x15(%rip) # 60 <skvm-jit-884702985+0x20>
75: c4 e2 7d 18 0d e6 ff ff ff vbroadcastss -0x1a(%rip),%ymm1 # 64 <skvm-jit-884702985+0x24>
7e: c4 e2 7d 18 15 e1 ff ff ff vbroadcastss -0x1f(%rip),%ymm2 # 68 <skvm-jit-884702985+0x28>
There are 3 vbroadcastss instructions here, each starting with c4 e2 7d
18, but the first has been disassembled as if its c4 were part of the
last data entry (0xff00ff00) as inc %esp.
Probably not a big deal for now, particularly since those vbroadcastss
are all outside the loop and never show up on a profile. If it gets too
confusing I think we can dump the programs starting from the beginning
of the code instead of from the data; we won't be able to inspect the
data, but everything should disassemble perfectly.
Change-Id: I0cc864359fd0740fc026070eaf2b6cb130783a57
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/222574
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
The encoding kind of all goes through the same paths,
as the three argument instructions, but like the nursery
rhyme when there are only two they kind of all roll over
and the op-extension hops into the bed.
vpermq is the first place we need to set the W bit
to indicate a 64-bit lane operation, so a little
minimal plumbing for that. It takes its arguments
a little differently too, passing dst where you'd
expect, the source where we'd pass y, and requiring
us to pass literal 0000 for the vvvv bits in VEX
(inverted as normal to literal 1111).
Change-Id: I91a4cd1b316eb908992631ce8b2cb3c62078e8c6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/222565
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
- 32x8 i32 add,sub,mul
- add I32_Naive bench/test builder to get better i32 mul coverage
- minor refactoring all over
Change-Id: I13cc19ff37a2da0bcff289ba51baac08f456d6c5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/222485
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This is a reland of a224fc1105
Changes since original:
- switch fJIT_K to less error prone fJITMask
- guard fJIT Assembler Program member with SKVM_JIT
Not really sure why the mips64el-Debug bot's compiler is crashing;
it does at least make sense to crash where it does... the file
includes SkOpts.h which includes SkVM.h.
If no reasonable code transformation can get it working again
I'll remove the bot. The -Release version is fine, and mips64el
is one of those things I'd happily flush if it blocks progress.
In this end I think all this SKVM_JIT and Xbyak stuff should
go away and make things simple again, hopefully too simple to
crash GCC. :|
Original change's description:
> extract Assembler so it can be tested
>
> And start documenting some structs we'll need
> to replace xbyak.
>
> Change-Id: I21c91642799a54e10af85afc8edbe12a9b4aa062
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/221644
> Reviewed-by: Herb Derby <herb@google.com>
> Commit-Queue: Mike Klein <mtklein@google.com>
Cq-Include-Trybots: skia.primary:Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Release-All-SK_CPU_LIMIT_SSE2,Build-Debian9-GCC-mips64el-Debug
Change-Id: I6d7c27bc758b23c164ee67067cdfacc291e289fc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/221983
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
This reverts commit a224fc1105.
Reason for revert: breaking x86-64 bots without AVX2, e.g. Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Release-All-SK_CPU_LIMIT_SSE2
Original change's description:
> extract Assembler so it can be tested
>
> And start documenting some structs we'll need
> to replace xbyak.
>
> Change-Id: I21c91642799a54e10af85afc8edbe12a9b4aa062
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/221644
> Reviewed-by: Herb Derby <herb@google.com>
> Commit-Queue: Mike Klein <mtklein@google.com>
TBR=mtklein@google.com,herb@google.com
Change-Id: Ie90d57f66e4d45f94db4ab4f485155533faddae1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/221655
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
And start documenting some structs we'll need
to replace xbyak.
Change-Id: I21c91642799a54e10af85afc8edbe12a9b4aa062
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/221644
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Any time we implement a Program::Instruction with multiple low-level
operations, we risk overwriting any arguments that alias the
destination.
This is why the _I32 tests are failing, mad_unorm8 where d == x. We
want (x*y+x)/256+z, but end up calculating (x*y+x*y)/256+z when x == d.
We could fix this by never allowing any arguments to alias any
destinations, but most instructions don't have this problem, and doing
that blindly would bloat the register count significantly.
We could fix this by knowing which Ops may be prone to aliasing in any
backend, but I find that somewhat error prone and also a little
abstraction- level-violatey. I would have thought, for instance, that
the mad_f32 Op might be vulnerable here, but it's actually not... in any
situation where there is aliasing, we actually lower it to a single
vfmadd instruction, never mul-then-add.
This sort of aliasing issue is going to keep coming back up again and
again, especially with 2-argument architectures like SSE. Luckily it's
trivially easy to fix by reserving a single tmp register to use as the
result of all but the final instructions.
The interpreter is safe because all its switch cases are single r(d) =
... statements. The right hand sides are evaluated before anything is
written back to a destination register slot. Had it been written a
little differently, it could have easily had this same aliasing issue.
Change-Id: I996392ef6af48268238ecae4a97d3bf3b4fba002
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/220600
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
The mask-only special case for extract is wrong...
it never looked it its input!
This not only makes things correct-er, but oddly it also
makes them faster by breaking inter-loop data dependencies.
Disable tests for _I32... they're actually still broken
because of a much more systemic flaw in how I've evaluated
programs. The _F32 and _I32_SWAR JIT code and all interpreted
code is just getting lucky. o_O
While here, update the I32_SWAR code to use the same math as I32,
(x*y+x)/256 for unorm8 mul. This just helps keep me sane.
Change-Id: I1acc09adb84c426fca4b2be5ca8c2d46d9678dd8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/220577
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Herb Derby <herb@google.com>
At head we're redoing any n<8 tail from the start,
not continuing from (n/8)*8 like we'd want.
Change-Id: I1a3d24cdffc843bbe6f3e01a163b6e3a20fdd0ca
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/220556
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
I used to have a dump of the value program before it was
translated to registers, but it went away a while ago.
This restores it.
Change-Id: I9b8bfcb124843cad4b0dc44bdf0a03e95a0c83d8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/219757
Commit-Queue: Mike Klein <mtklein@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Can sometimes be hard to know what's going on
on the bots without a little bit more debug help.
Change-Id: Ie556a8de88349170e9d9e44c16098223442838a2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/218316
Auto-Submit: Mike Klein <mtklein@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Eliminate the duplicate functionality,
and better testing for the bench builders.
Change-Id: If20e52107738903f854aec431416e573d7a7d640
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/218041
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
- keep expectations in resources/
- overwrite automatically if needed
so we can see the diff in Git
Change-Id: I2486b127ebcc7f40332fd0462e38b1af04d3e32b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/218038
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
Change-Id: I5aa5f789180b9caba70952cc60a2e9bbcf3b5a97
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/217983
Commit-Queue: Mike Klein <mtklein@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
With all the thinking around a stack-based interpreter,
I figured I'd sketch out some ideas for a register VM too.
I kind of have the hunch that this is the direction that
will actually let us replace large amounts of Skia's CPU
backend with an efficient interpreter or JIT.
Change-Id: Ia2b5ba4a3fc27556f5b6ba95cd1ace46d3217403
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/216665
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
