Stream: git-wasmtime

Topic: wasmtime / PR #2395 Add support for brff/brif and icmp_sp...

Wasmtime GitHub notifications bot (Nov 11 2020 at 21:58):

cfallin opened PR #2395 from lucet-x64-support to main:

lucetc currently almost, but not quite, works with the new x64
backend; the only missing piece is support for the particular
instructions emitted as part of its prologue stack-check.

We do not normally see brff, brif, or ifcmp_sp in CLIF generated by
cranelift-wasm without the old-backend legalization rules, so these
were not supported in the new x64 backend as they were not necessary for
Wasm MVP support. Using them resulted in an unimplemented!() panic.

This PR adds support for brff and brif analogously to how AArch64
implements them, by pattern-matching the ifcmp / ffcmp directly.
Then ifcmp_sp is a straightforward variant of ifcmp.

Along the way, this also removes the notion of "fallthrough block" from
the branch-group lowering method; instead, fallthrough instructions
are handled as normal branches to their explicitly-provided targets,
which (in the original CLIF) match the fallthrough block. The reason for
this is that the block reordering done as part of lowering can change
the fallthrough block. We were not using fallthrough instructions in
the output produced by cranelift-wasm, so this, too, was not
previously caught.

With these changes (and with #2394), the lucetc crate in Lucet
passes all tests with the x64 feature-flag added to its
cranelift-codegen dependency.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Nov 11 2020 at 21:58):

cfallin requested abrown and iximeow for a review on PR #2395.

Wasmtime GitHub notifications bot (Nov 11 2020 at 21:58):

cfallin requested abrown and iximeow for a review on PR #2395.

Wasmtime GitHub notifications bot (Nov 12 2020 at 19:35):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 12 2020 at 19:35):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 12 2020 at 19:35):

abrown created PR Review Comment:

Here's the applicable fallthrough documentation: "Fall through to the next block. This is the same as jump, except the destination block must be the next one in the layout."

Wasmtime GitHub notifications bot (Nov 12 2020 at 19:35):

abrown created PR Review Comment:

Hm... so we _have_ to jump for both sides of the branch, right? I would hope this doesn't affect performance too much (it's an unconditional jump) but it basically means that the Cranelift fallthrough instruction does not have the meaning Cranelift users might expect (I would expect no instruction would be necessary because fallthrough would imply the blocks are contiguous). I would suggest we resolve this in some way:

• perhaps add an issue to figure out how to preserve the fallthrough assumptions in the new backend?
• perhaps remove fallthrough, forcing users to use an unconditional jump instead?

Wasmtime GitHub notifications bot (Nov 12 2020 at 19:41):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Nov 12 2020 at 19:41):

bjorn3 created PR Review Comment:

fallthrough mainly exists for the old backend. The old backend automatically replaces unconditional jumps to the next block with it.

Wasmtime GitHub notifications bot (Nov 12 2020 at 20:04):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 12 2020 at 20:04):

cfallin created PR Review Comment:

Indeed, I think that what this patch does is semantically correct (if the code-generator follows the rules, then we will generate correct machine code), and in fact even correct for a superset of defined behavior (if the code-gen puts the blocks in the wrong order, we will still use the named target of fallthrough), but loses the block-ordering hint because our block-ordering logic in BlockLoweringOrder does its own DFS.

So I think the right thing to do here would be to (i) define fallthrough to be a hint, i.e., equivalent to a jump but expressing a desire to place the named block next in final code (this is compatible with current definition), and (ii) use that hint in the lowering logic. I can create an issue for this, as @abrown suggests. Thanks!

Wasmtime GitHub notifications bot (Nov 12 2020 at 20:10):

cfallin merged PR #2395.

Last updated: Nov 22 2024 at 17:03 UTC