Stream: git-wasmtime

Topic: wasmtime / PR #4725 Print constants in a comment in CLIF ...


view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 20:38):

jameysharp requested fitzgen for a review on PR #4725.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 20:38):

jameysharp opened PR #4725 from print-consts to main:

When trying to read generated CLIF, it's nice to be able to see at a glance that some of the operands are defined by iconst instructions, without having to go find each operand's definition manually.

@afonso360 suggested this in #4721 (and @bjorn3 seconded the idea) but I think it's a good idea independent of that issue.

I'm not sure if this is the right implementation, though: I don't understand the intent of the FuncWriter trait well enough to know if adding this to the shared write_operands function is the best place. Perhaps only some implementations of FuncWriter should do this?

Because of that uncertainty, I haven't fixed up any of the tests which fail due to not expecting these extra comments. So I expect that CI should currently be failing.

<!--

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 (Aug 16 2022 at 20:38):

jameysharp requested cfallin for a review on PR #4725.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 21:04):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 21:26):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 21:26):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 21:26):

afonso360 created PR review comment:

What do you think about expanding this to UnaryIeee{32, 64} and UnaryBool?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 21:26):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 21:27):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 23:07):

jameysharp updated PR #4725 from print-consts to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 23:09):

jameysharp updated PR #4725 from print-consts to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 23:12):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 23:12):

jameysharp created PR review comment:

Okay, done! That still leaves vconst but that's more complicated since those constants aren't inline in the instruction. I don't feel like figuring out how that works right now since it was enough of a pain fixing the tests for these changes already. I'd be happy to review a PR on top of this one if somebody feels like taking that on though.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 23:12):

jameysharp has marked PR #4725 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 23:15):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 23:34):

jameysharp updated PR #4725 from print-consts to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 23:36):

jameysharp created PR review comment:

I changed my mind: it's good enough to just print which element of the constant pool is in a vector-typed value, and that was easy to do. I think that might actually be more useful than printing giant vector literals. So now there's output for vconst as well.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2022 at 23:36):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2022 at 16:00):

jameysharp merged PR #4725.


Last updated: Dec 23 2024 at 12:05 UTC