Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PCC: check facts on loaded and stored values, infer facts where needed, and add a basic vmctx/memory example. #7231

Merged
merged 5 commits into from
Oct 13, 2023

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Oct 12, 2023

This PR incorporates a few more steps in PCC's development:

  • We now check that the fact, if any, provided for a field in a memory type supports (subsumes) the claimed fact on the result of a load from that field. Likewise, we check that the fact on any value stored to a field supports the field's fact.
  • We check that facts are preserved across control-flow edges by validating that branch arguments' facts subsume blockparams' facts.
  • For integer-typed values, when no other fact is present and one is needed during validation, we infer a minimal fact based on the type's full range.

With all of these together, we can validate a simple "mock vmctx" example that looks a lot like what static memory accesses in Wasmtime do:

test compile
set enable_pcc=true
target aarch64

;; Equivalent to a Wasm `i64.load` from a static memory.
function %f0(i64, i32) -> i64 {
    ;; mock vmctx struct:
    mt0 = struct 8 { 0: i64 readonly ! mem(mt1, 0) }
    ;; mock static memory: 4GiB range, plus 2GiB guard
    mt1 = memory 0x1_8000_0000

block0(v0 ! mem(mt0, 0): i64, v1: i32):
    ;; Compute the address: base + offset. Guard region (2GiB) is
    ;; sufficient for an 8-byte I64 load.
    v2 ! mem(mt1, 0) = load.i64 checked v0+0    ;; base pointer
    v3 ! max(64, 0xffff_ffff) = uextend.i64 v1  ;; offset
    v4 ! mem(mt1, 0xffff_ffff) = iadd.i64 v2, v3
    v5 = load.i64 checked v4
    return v5
}

In theory, once we propagate facts during aegraph rewriting (we don't currently; opts are off in all tests), this should be enough for end-to-end checking of static memories by emitting the right facts in cranelift-wasm. Those two steps (opts, then Wasm frontend) are next!

Co-authored-by: Nick Fitzgerald fitzgen@gmail.com

cfallin and others added 3 commits October 12, 2023 12:19
Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
@cfallin cfallin requested a review from a team as a code owner October 12, 2023 19:24
@cfallin cfallin requested review from abrown and fitzgen and removed request for a team and abrown October 12, 2023 19:24
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

cranelift/filetests/filetests/pcc/fail/vmctx.clif Outdated Show resolved Hide resolved
cranelift/filetests/filetests/pcc/succeed/vmctx.clif Outdated Show resolved Hide resolved
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. labels Oct 12, 2023
- Allow any fact at all to subsume a trivially-true range fact that
  states a bitslice is within the maximum range for its width.

- Clamp the maximum value of a bitslice at the maximum value for the
  slice's width.
@cfallin
Copy link
Member Author

cfallin commented Oct 13, 2023

The test failures exposed two issues surfaced by the new "default facts" (e.g., width-8 value is always <= 255), so I had to add a bit more reasoning. @fitzgen mind taking a look at the last commit?

@cfallin cfallin added this pull request to the merge queue Oct 13, 2023
Merged via the queue into bytecodealliance:main with commit 2e7f941 Oct 13, 2023
18 checks passed
@cfallin cfallin deleted the pcc-memtypes-load branch October 13, 2023 01:36
@cfallin
Copy link
Member Author

cfallin commented Oct 13, 2023

Ah -- automerge was still on and this went in with your r+, @fitzgen; still happy to take feedback on the approach in the last commit if you have any!

Comment on lines +233 to +239
// Any value on the LHS subsumes a minimal (always-true)
// fact about the max value of a given bitwidth on the
// RHS: e.g., no matter the value, the bottom 8 bits will
// always be <= 255.
(_, Fact::ValueMax { bit_width, max }) if *max == max_value_for_width(*bit_width) => {
true
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is technically wrong but it feels like a footgun to allow a Mem fact to subsume a minimal ValueMax fact.

Would it be a problem to limit this to ValueMax LHSes? I guess that is what would happen if this case wasn't here, due to the match arm just below this one. Can you explain in more detail why we need this kind of cross-fact-kind subsumption then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that that narrower rule is indeed sufficient, at least for our test cases. The new rule is still necessary (not covered by the rule below) because of the remaining difference: it doesn't require matching bit-widths.

All of this is a bit of awkward fallout of the way default facts are working now though, so I'm going to see if I can try again to eliminate them and make subsume take the type instead. If not, I'll update as you suggest!

cfallin added a commit to cfallin/wasmtime that referenced this pull request Oct 16, 2023
This removes the need for the awkward "max-range fact is subsumed by
anything" rule noted by @fitzgen in [this
comment](bytecodealliance#7231 (comment)).
It also makes checking a little more efficient and logically clear, as
only the facts that the frontend/producer added are verified, rather
than all default facts as well.
github-merge-queue bot pushed a commit that referenced this pull request Oct 17, 2023
This removes the need for the awkward "max-range fact is subsumed by
anything" rule noted by @fitzgen in [this
comment](#7231 (comment)).
It also makes checking a little more efficient and logically clear, as
only the facts that the frontend/producer added are verified, rather
than all default facts as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants