wasmtime / PR #7180 PCC: add semantics for core add/shift... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #7180 PCC: add semantics for core add/shift...

Wasmtime GitHub notifications bot (Oct 06 2023 at 22:33):

cfallin opened PR #7180 from cfallin:pcc-add-shift-extend to bytecodealliance:main:

This PR adds verification of facts on values produced by adds, shifts, and extends on AArch64, handling the various combination instructions (adds with builtin extends or shifts, for example), and also adds verification of all addressing modes, including those with builtin extends and shifts.

It also splits the test suite into"succeed" and "fail" sets, and provides cases that PCC should catch.

This is stacked on top of #7165; only the last commit is new. I'll rebase once that PR merges.

Wasmtime GitHub notifications bot (Oct 06 2023 at 22:33):

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

Wasmtime GitHub notifications bot (Oct 06 2023 at 22:33):

cfallin requested elliottt for a review on PR #7180.

Wasmtime GitHub notifications bot (Oct 06 2023 at 22:33):

cfallin requested alexcrichton for a review on PR #7180.

Wasmtime GitHub notifications bot (Oct 06 2023 at 22:33):

cfallin requested wasmtime-core-reviewers for a review on PR #7180.

Wasmtime GitHub notifications bot (Oct 06 2023 at 22:33):

cfallin requested fitzgen for a review on PR #7180.

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:20):

cfallin updated PR #7180.

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:20):

cfallin edited PR #7180:

This PR adds verification of facts on values produced by adds, shifts, and extends on AArch64, handling the various combination instructions (adds with builtin extends or shifts, for example), and also adds verification of all addressing modes, including those with builtin extends and shifts.

It also splits the test suite into"succeed" and "fail" sets, and provides cases that PCC should catch.

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:21):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:21):

cfallin created PR review comment:

This was an imprecision in the initial impl -- we had returned the "full range" fact (correct but not useful) rather than the thing we actually computed.

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:39):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:39):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:39):

fitzgen created PR review comment:

Let's try to avoid as here and elsewhere.

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:39):

fitzgen created PR review comment:

And we could reuse that factored shl method here :)

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:39):

fitzgen created PR review comment:

Can we factor this out to a ctx.shl method?

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:39):

fitzgen created PR review comment:

Can we do a.checked_mul(b).unwrap() to assert that this will not overflow?

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:39):

fitzgen created PR review comment:

It would be nice to (eventually) have expected error messages that we check against for each of these failures.

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:39):

fitzgen created PR review comment:

Do we know we won't ever look up facts for the aliased vreg anymore? That is, is it really safe to take rather than clone?

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:39):

fitzgen created PR review comment:

get_fact_or_fail is called a whooooole lot. Maybe we can just call it get_fact?

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:50):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2023 at 23:50):

cfallin created PR review comment:

The lookup follows the alias, so querying the alias will get the fact as well -- we're moving it here so if the aliaser had a fact and the aliasee didn't, the fact gets preserved (that's the common case with the way ISLE lowering works -- we alias the original reg to the new temp the rule returned).