Wasmtime GitHub notifications bot (Jun 18 2020 at 18:33): Wasmtime GitHub notifications bot (Jun 18 2020 at 18:33):cfallin opened PR #1900 from clarify-lowering-docs to master:
From discussion with Julian and Ben, this PR makes a few documentation-
and naming-level changes (no functionality change):
Document that the
LowerCtx-provided output register can be used as a
scratch register during the lowered instruction sequence before
placing the final result in it.Rename
input_to_*helpers in the AArch64 backend to
put_input_in_*, emphasizing that these are side-effecting helpers
that potentially generate code (e.g., sign/zero-extensions) to ensure
an input value is in a register.<!--
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 (Jun 18 2020 at 18:33):cfallin requested bnjbvr and julian-seward1 for a review on PR #1900.
Wasmtime GitHub notifications bot (Jun 18 2020 at 18:33):cfallin requested bnjbvr and julian-seward1 for a review on PR #1900.
Wasmtime GitHub notifications bot (Jun 18 2020 at 19:21):cfallin updated PR #1900 from clarify-lowering-docs to master:
From discussion with Julian and Ben, this PR makes a few documentation-
and naming-level changes (no functionality change):
Document that the
LowerCtx-provided output register can be used as a
scratch register during the lowered instruction sequence before
placing the final result in it.Rename
input_to_*helpers in the AArch64 backend to
put_input_in_*, emphasizing that these are side-effecting helpers
that potentially generate code (e.g., sign/zero-extensions) to ensure
an input value is in a register.<!--
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 (Jun 19 2020 at 05:05):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jun 19 2020 at 05:05):julian-seward1 created PR Review Comment:
Indeed, IIUC,
it is guaranteed not to overlap with any of the inputs of either this or any other IR instruction, yes?
Wasmtime GitHub notifications bot (Jun 19 2020 at 05:05):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jun 22 2020 at 12:44):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 22 2020 at 12:44):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 22 2020 at 12:44):bnjbvr created PR Review Comment:
This output IR inst could be the input to another IR inst, right? And we're only concerned about usage local to the current IR instruction here. Maybe I'm missing something here?
Wasmtime GitHub notifications bot (Jun 22 2020 at 12:44):bnjbvr created PR Review Comment:
Thanks for this renaming which clarifies the side-effects happening here indeed.
As an alternative, as demonstrated by the changes in this PR, it seems that the narrowing is the edge case, and not the usual thing to do.
Two propositions:
- either we could add another helper function
get_input(ctx, input)which really reads the input and does no side-effect? Callers ofput_input_in_regwithNarrowMode::Nonecould call this function instead.- or we could just remove the NarrowMode parameter all the time, and having another helper function that does the narrowing, separating the different responsibilities more clearly? (I hope to implement it this way on the x64 backend, if possible.)
Wasmtime GitHub notifications bot (Jun 23 2020 at 16:54):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jun 23 2020 at 16:54):cfallin created PR Review Comment:
Yes, as @bnjbvr said, the guarantee is only in the context of the current instruction being lowered (since if it were never used by any other IR instruction either, the computation would be dead).
Wasmtime GitHub notifications bot (Jun 23 2020 at 16:57):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jun 23 2020 at 16:57):cfallin created PR Review Comment:
Sure, I would be fine with the latter; perhaps we can consider this in the context of refactoring to share helpers with the x64 backend? (I'm thinking that e.g. we could define a trait to describe codegen for value widening and then plug that into a generic helper, for example. Similar for codegenning constants. Part of a "libMachBackend" of sorts as an optional layer on top of the lowering context API.)
Wasmtime GitHub notifications bot (Jun 23 2020 at 16:59):cfallin merged PR #1900.
Wasmtime GitHub notifications bot (Jun 24 2020 at 09:09):bnjbvr created PR Review Comment:
Good idea!
Wasmtime GitHub notifications bot (Jun 24 2020 at 09:09):bnjbvr submitted PR Review.
Last updated: Nov 22 2024 at 17:03 UTC