Wasmtime GitHub notifications bot (Aug 16 2022 at 20:38): Wasmtime GitHub notifications bot (Aug 16 2022 at 20:38):jameysharp requested fitzgen for a review on PR #4725.
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
iconstinstructions, 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
FuncWritertrait well enough to know if adding this to the sharedwrite_operandsfunction is the best place. Perhaps only some implementations ofFuncWritershould 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.
[ ] 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 (Aug 16 2022 at 20:38):jameysharp requested cfallin for a review on PR #4725.
Wasmtime GitHub notifications bot (Aug 16 2022 at 21:04):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 16 2022 at 21:26):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Aug 16 2022 at 21:26):afonso360 submitted PR review.
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}andUnaryBool?
Wasmtime GitHub notifications bot (Aug 16 2022 at 21:26):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (Aug 16 2022 at 21:27):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (Aug 16 2022 at 23:07):jameysharp updated PR #4725 from print-consts to main.
Wasmtime GitHub notifications bot (Aug 16 2022 at 23:09):jameysharp updated PR #4725 from print-consts to main.
Wasmtime GitHub notifications bot (Aug 16 2022 at 23:12):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Aug 16 2022 at 23:12):jameysharp created PR review comment:
Okay, done! That still leaves
vconstbut 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.
Wasmtime GitHub notifications bot (Aug 16 2022 at 23:12):jameysharp has marked PR #4725 as ready for review.
Wasmtime GitHub notifications bot (Aug 16 2022 at 23:15):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Aug 16 2022 at 23:34):jameysharp updated PR #4725 from print-consts to main.
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
vconstas well.
Wasmtime GitHub notifications bot (Aug 16 2022 at 23:36):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Aug 17 2022 at 16:00):jameysharp merged PR #4725.
Last updated: Oct 23 2024 at 20:03 UTC