Stream: git-wasmtime

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


view this post on Zulip 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.

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

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 02:30):

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

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 02:49):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 02:49):

cfallin submitted PR review.

view this post on Zulip 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:

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

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 02:52):

cfallin submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 03:26):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 03:26):

elliottt submitted PR review.

view this post on Zulip 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:

view this post on Zulip 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.

view this post on Zulip 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)?

view this post on Zulip 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?

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 03:27):

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

view this post on Zulip 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({} || {})?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 03:28):

elliottt submitted PR review.

view this post on Zulip 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...

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 03:45):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 04:32):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 04:37):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 04:41):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 04:45):

elliottt submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 17:08):

elliottt has marked PR #4545 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 29 2022 at 17:18):

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

view this post on Zulip 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 :)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 06:41):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 18:06):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 18:24):

elliottt merged PR #4545.


Last updated: Dec 23 2024 at 12:05 UTC