Stream: git-wasmtime

Topic: wasmtime / PR #4545 x64: Migrate trapif and trapff to ISLE

Wasmtime GitHub notifications bot (Jul 29 2022 at 00:28):

elliottt edited PR #4545 from trevor/x64-lower-trapif to main:

Migrate the trapif instruction to ISLE, and extract the lowering of icmp for scalars to a utility function. The icmp extraction will be helpful for future instruction migrations, as its uses mirror the existing emit_cmp instruction in the rust implementation.

I'd recommend reviewing by commit, and turning off whitespace in the diff.

<!--

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 (Jul 29 2022 at 02:30):

elliottt updated PR #4545 from trevor/x64-lower-trapif to main.

Wasmtime GitHub notifications bot (Jul 29 2022 at 02:49):

cfallin created PR review comment:

here and elsewhere, we shouldn't do the side-effecting thing and invoke trap_if_bool just to throw the result away; we should return it. This can work similarly to other ops that don't return values most likely -- see e.g. stores, with the (side_effect ...) ctor.

Wasmtime GitHub notifications bot (Jul 29 2022 at 02:49):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2022 at 02:49):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2022 at 02:49):

cfallin created PR review comment:

I think this could work a little better if we did the following:

• The AlreadyExistingFlags enum arm has no args -- we don't actually hold onto the flags Value, it's just a sentinel representing "whatever's in the machine flags register";
• We write a helper in ISLE that does something like:
  lisp (decl flags_to_producesflags (Value) ProducesFlags) (rule (flags_to_producesflags flags) (mark_value_used flags) (ProducesFlags.AlreadyExistingFlags))

then we don't need the distinct case when emitting.

Wasmtime GitHub notifications bot (Jul 29 2022 at 02:52):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2022 at 02:52):

cfallin created PR review comment:

And just to write down a thought I had and then dismissed: I had considered whether it made sense to make the above an auto-conversion, but I think that's too dangerous; if makes more sense to require an explicit action here since we're essentially asserting that flags is a flags value.

... or, perhaps we do:

(rule (flags_to_producesflags (has_type $FLAGS flags))
  ...)

then we are actually asserting it's a flags value, and the verifier will ensure this corresponds to the current flags (there is only one live flags at a time), and so then this is safe and we can make it an auto-conversion. That removes all footguns from the lowerings I think.

Wasmtime GitHub notifications bot (Jul 29 2022 at 03:26):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2022 at 03:26):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2022 at 03:26):

elliottt created PR review comment:

I agree that emit_consumer feels like a bit of a hack, and I'd be happy to use an alternative. The solution to bump the refcount manually when consuming the iadd_ifcout seems good to me :+1:

Wasmtime GitHub notifications bot (Jul 29 2022 at 03:26):

elliottt created PR review comment:

This is a change from the previous lowering of icmp for comparisons with I128 arguments: instead of ending with a single x64_and, we now produce a test instruction. The lowering for icmp will now include an additional setcc as a result, for I128 comparisons.

Wasmtime GitHub notifications bot (Jul 29 2022 at 03:26):

elliottt created PR review comment:

I'm not sure what value we would return here -- TrapIf and friends have no outputs. Are you suggesting that trap_if_bool should return an InstOutput and always return (output_none)?

Wasmtime GitHub notifications bot (Jul 29 2022 at 03:26):

elliottt created PR review comment:

Would we need to select the second output of flags for that assertion to work, or am I missing something about how the outputs are accessed?

Wasmtime GitHub notifications bot (Jul 29 2022 at 03:26):

elliottt created PR review comment:

Ah of course: we can return a SideEffectNoResult.Inst2 instead. I'll make that change, it's much cleaner than relying on the effects currently.

Wasmtime GitHub notifications bot (Jul 29 2022 at 03:27):

elliottt updated PR #4545 from trevor/x64-lower-trapif to main.

Wasmtime GitHub notifications bot (Jul 29 2022 at 03:28):

elliottt created PR review comment:

I'm not sold on this pretty-printing format. Do you have ideas on how better to convey what's going on? Maybe something like j({} || {})?

Wasmtime GitHub notifications bot (Jul 29 2022 at 03:28):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2022 at 03:45):

cfallin created PR review comment:

(I'll look at the rest in the detail tomorrow but one thought here: )

This is actually kind of a fun historical oddity: when initially implementing the new backends we kept the pretty-printing format to something that could actually feed into GNU as. The first "hello world" (fib function actually) on the aarch64 backend was a thing I captured from the trace logs and then assembled and linked manually. Since then we've sort of drifted from that and the pretty-printing just has to be "comprehensible to a deeply tired debugging human"; so something that clarifies here is welcome :-)

I might even go so far as trap_if_or CC, CC or somesuch...

Wasmtime GitHub notifications bot (Jul 29 2022 at 03:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2022 at 04:32):

elliottt updated PR #4545 from trevor/x64-lower-trapif to main.

Wasmtime GitHub notifications bot (Jul 29 2022 at 04:37):

elliottt updated PR #4545 from trevor/x64-lower-trapif to main.

Wasmtime GitHub notifications bot (Jul 29 2022 at 04:41):

elliottt updated PR #4545 from trevor/x64-lower-trapif to main.

Wasmtime GitHub notifications bot (Jul 29 2022 at 04:45):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2022 at 04:45):

elliottt created PR review comment:

This part of the comment seems like it would be more helpful by the lower_fcmp_bool function above.

Wasmtime GitHub notifications bot (Jul 29 2022 at 17:08):

elliottt has marked PR #4545 as ready for review.

Wasmtime GitHub notifications bot (Jul 29 2022 at 17:18):

elliottt updated PR #4545 from trevor/x64-lower-trapif to main.

Wasmtime GitHub notifications bot (Aug 01 2022 at 06:41):

elliottt created PR review comment:

I realized that I was forgetting that the dependency in trapif will explicitly be for the flags output of iadd_ifcout, so I was indeed missing something :)

Wasmtime GitHub notifications bot (Aug 01 2022 at 06:41):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:24):

elliottt merged PR #4545.

Last updated: Nov 22 2024 at 17:03 UTC