cfallin requested fitzgen for a review on PR #7281.
cfallin opened PR #7281 from cfallin:pcc-owl
to bytecodealliance:main
:
This needed a bit more inference / magic than I was hoping for at first,
specifically around constants and adds. Some instructions can now
generate facts on their output registers, even if not stated. This
breaks away from the "breadcrumbs" idea, but seems to be the most
practical solution to a general problem that we have mini-lowering steps
in various places without careful preservation of PCC facts. Two
particular aspects:
Constants: amodes on aarch64 can decompose into new
constant-generation instructions, and we need precise ranges on those
to properly check them. To avoid making the ISLE rules nightmarish,
it's best to reuse the existing semantics definitions of the Add* ALU
insts, and add a few rules for MovK/MovZ/MovN.Adds: similarly, amodes decompose into de-novo add instructions with
no facts. To handle this, there's now a notion of "propagating" facts
that cause an instruction with a propagating fact on the input to
generate a fact on the output.Together, these heuristics mean that we'll eagerly generate a fact for
mem(mt0, 0, 0) + 8 -> mem(mt0, 8, 8)
, but we won't, for example,
generate ranges on every single integer operation.With these changes and a few other misc fixes, this PR can now check a
nontrivial "hello world" Wasm on aarch64 down to the machine-code level:$ target/release/wasmtime compile -C enable-pcc=y ../wasm-tests/helloworld-rs.wasm
(This builds on #7280, and I also intend to clean it up a bit, so creating as a draft for now)
cfallin edited PR #7281:
This needed a bit more inference / magic than I was hoping for at first,
specifically around constants and adds. Some instructions can now
generate facts on their output registers, even if not stated. This
breaks away from the "breadcrumbs" idea, but seems to be the most
practical solution to a general problem that we have mini-lowering steps
in various places without careful preservation of PCC facts. Two
particular aspects:
Constants: amodes on aarch64 can decompose into new
constant-generation instructions, and we need precise ranges on those
to properly check them. To avoid making the ISLE rules nightmarish,
it's best to reuse the existing semantics definitions of the Add* ALU
insts, and add a few rules for MovK/MovZ/MovN.Adds: similarly, amodes decompose into de-novo add instructions with
no facts. To handle this, there's now a notion of "propagating" facts
that cause an instruction with a propagating fact on the input to
generate a fact on the output.Together, these heuristics mean that we'll eagerly generate a fact for
mem(mt0, 0, 0) + 8 -> mem(mt0, 8, 8)
, but we won't, for example,
generate ranges on every single integer operation.With these changes and a few other misc fixes, this PR can now check a
nontrivial "hello world" Wasm on aarch64 down to the machine-code level:$ target/release/wasmtime compile -C enable-pcc=y ../wasm-tests/helloworld-rs.wasm # completes successfully
(This builds on #7280, and I also intend to clean it up a bit, so creating as a draft for now)
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
Reminder to undo this.
fitzgen created PR review comment:
I thought join was usually union and meet was usually intersection? Eg when you have a powerset lattice, usually you join and union. So in that case, this is the usual lattice meet operation.
I guess what is unusual is that we are able to meet rather than join because we know both facts must be true, rather than the typical situation in a static analysis where one is generalizing over assumptions.
fitzgen created PR review comment:
Nitpick: kinda expect a
bool
result from anis_foo
method. Maybe we can name thisas_const
?
fitzgen created PR review comment:
Maybe debug assert that the value fits in the bit width?
fitzgen created PR review comment:
Should we infer facts even when our merged facts conflict? That seems like papering over fact-producer bugs.
I guess I'd expect merging to handle the
(None, None)
case explicitly to return an inferred fact, and then here we would always returnConflict
.
fitzgen created PR review comment:
Probably don't want to land these logs I'm assuming.
fitzgen created PR review comment:
Seems like we are getting pretty wordy here and the
check_output
helper isn't helping as much as it could.We already have to pass in the operands to check if they need propagation, so can we have the helper automatically get their facts and pass them to the closure? This would mean the closure doesn't need to close over
r{n,m}
which means we don't need separate bindings where we eagerly copy just to avoid borrow issues.We could pass the operands by slice or have unop and binop helpers to make things a little nicer.
So ultimately, this would end up looking something like this (closure types annotated just for clarity of the idea):
let size = *size; let extendop = *extendop; check_binop(&ctx, vcode, rd.to_reg(), rn, rm, |vcode, rn: Fact, rm: Fact| { let rm_extended = fail_if_missing(extend_fact(&ctx, rm, extendop))?; fail_if_missing(ctx.add(&rn, &rm_extended, size.bits().into())) })
which is a lot less verbose
fitzgen created PR review comment:
This should be an actual error, not a panic.
fitzgen created PR review comment:
Seems like we could make a
Fact::constant(bit_width, value)
method and then use that incheck_constant
as well and it would be a good place to deduplicate and provide a single place to debug assert that the value fits in the bits and all that.
fitzgen created PR review comment:
All this
if pcc { attach_fact() }
stuff is starting to feel like it could use a helper as well:let add_fact = |pos, val, fact| { if pcc { pos.func.dfg.facts[val] = fact; } };
which should clean up the attachment sites a bit and lessen cognitive overhead associated with the extra control flow of
if pcc
everywhere.
cfallin updated PR #7281.
cfallin submitted PR review.
cfallin created PR review comment:
Done!
cfallin submitted PR review.
cfallin created PR review comment:
Done!
cfallin submitted PR review.
cfallin created PR review comment:
Discussed during live review just now; we landed on
intersect
instead to avoid convention issues :-)
cfallin submitted PR review.
cfallin created PR review comment:
It turns out we can remove this, empirically, so I did!
cfallin submitted PR review.
cfallin created PR review comment:
Done!
cfallin submitted PR review.
cfallin created PR review comment:
Done!
cfallin submitted PR review.
cfallin created PR review comment:
Refactored, thanks, that's a really nice cleanup!
cfallin submitted PR review.
cfallin created PR review comment:
Fixed!
cfallin submitted PR review.
cfallin created PR review comment:
Removed!
cfallin submitted PR review.
cfallin created PR review comment:
Fixed!
cfallin updated PR #7281.
cfallin has marked PR #7281 as ready for review.
cfallin requested wasmtime-compiler-reviewers for a review on PR #7281.
cfallin has enabled auto merge for PR #7281.
cfallin has disabled auto merge for PR #7281.
fitzgen submitted PR review:
Thanks!
fitzgen merged PR #7281.
Last updated: Dec 23 2024 at 12:05 UTC