wasmtime / PR #7281 PCC: draw the rest of the owl: fully-... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #7281 PCC: draw the rest of the owl: fully-...

Wasmtime GitHub notifications bot (Oct 18 2023 at 08:43):

cfallin requested fitzgen for a review on PR #7281.

Wasmtime GitHub notifications bot (Oct 18 2023 at 08:43):

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)

Wasmtime GitHub notifications bot (Oct 18 2023 at 08:43):

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)

Wasmtime GitHub notifications bot (Oct 18 2023 at 17:13):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 18 2023 at 17:13):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 18 2023 at 17:13):

fitzgen created PR review comment:

Reminder to undo this.

Wasmtime GitHub notifications bot (Oct 18 2023 at 17:13):

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.

Wasmtime GitHub notifications bot (Oct 18 2023 at 17:13):

fitzgen created PR review comment:

Nitpick: kinda expect a bool result from an is_foo method. Maybe we can name this as_const?

Wasmtime GitHub notifications bot (Oct 18 2023 at 17:13):

fitzgen created PR review comment:

Maybe debug assert that the value fits in the bit width?

Wasmtime GitHub notifications bot (Oct 18 2023 at 17:13):

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 return Conflict.

Wasmtime GitHub notifications bot (Oct 18 2023 at 17:13):

fitzgen created PR review comment:

Probably don't want to land these logs I'm assuming.

Wasmtime GitHub notifications bot (Oct 18 2023 at 17:13):

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

Wasmtime GitHub notifications bot (Oct 18 2023 at 17:13):

fitzgen created PR review comment:

This should be an actual error, not a panic.

Wasmtime GitHub notifications bot (Oct 18 2023 at 17:13):

fitzgen created PR review comment:

Seems like we could make a Fact::constant(bit_width, value) method and then use that in check_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.

Wasmtime GitHub notifications bot (Oct 18 2023 at 17:13):

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.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin updated PR #7281.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin created PR review comment:

Discussed during live review just now; we landed on intersect instead to avoid convention issues :-)

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin created PR review comment:

It turns out we can remove this, empirically, so I did!

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin created PR review comment:

Refactored, thanks, that's a really nice cleanup!

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin created PR review comment:

Fixed!

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin created PR review comment:

Removed!

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:06):

cfallin created PR review comment:

Fixed!

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:08):

cfallin updated PR #7281.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:08):

cfallin has marked PR #7281 as ready for review.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:08):

cfallin requested wasmtime-compiler-reviewers for a review on PR #7281.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:08):

cfallin has enabled auto merge for PR #7281.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:08):

cfallin has disabled auto merge for PR #7281.

Wasmtime GitHub notifications bot (Oct 19 2023 at 19:54):

fitzgen submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Oct 19 2023 at 20:40):

fitzgen merged PR #7281.

Last updated: Nov 22 2024 at 16:03 UTC