Wasmtime GitHub notifications bot (Jul 01 2020 at 23:33): Wasmtime GitHub notifications bot (Jul 01 2020 at 23:33):cfallin opened PR #1962 from aarch64-lowering-condbr to main:
In discussions with @bnjbvr, it came up that generating
OneWayCondBrs
with explicit, hardcoded PC-offsets as part of lowered instruction
sequences is actually unsafe, because the register allocator might
insert a spill or reload into the middle of our sequence. We were
careful about this in some cases but somehow missed that it was a
general restriction. Conceptually, all inter-instruction references
should be via labels at the VCode level; explicit offsets are only ever
known at emission time, and resolved by theMachBuffer.To allow the simple single-in, single-out local control flow that e.g.
trap checks require, without modifying the CFG (as seen by regalloc)
during lowering, this PR instead adds aCondSkippseudo-instruction
that conditionally skips a single embedded instruction. This is used to
skip around trap instructions in various places. It lowers to the same
condbr label ; trap ; label: ...sequence, but without the hardcoded
branch-target offset.<!--
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 01 2020 at 23:33):cfallin requested bnjbvr for a review on PR #1962.
Wasmtime GitHub notifications bot (Jul 01 2020 at 23:41):cfallin updated PR #1962 from aarch64-lowering-condbr to main:
In discussions with @bnjbvr, it came up that generating
OneWayCondBrs
with explicit, hardcoded PC-offsets as part of lowered instruction
sequences is actually unsafe, because the register allocator might
insert a spill or reload into the middle of our sequence. We were
careful about this in some cases but somehow missed that it was a
general restriction. Conceptually, all inter-instruction references
should be via labels at the VCode level; explicit offsets are only ever
known at emission time, and resolved by theMachBuffer.To allow the simple single-in, single-out local control flow that e.g.
trap checks require, without modifying the CFG (as seen by regalloc)
during lowering, this PR instead adds aCondSkippseudo-instruction
that conditionally skips a single embedded instruction. This is used to
skip around trap instructions in various places. It lowers to the same
condbr label ; trap ; label: ...sequence, but without the hardcoded
branch-target offset.<!--
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 02 2020 at 12:45):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jul 02 2020 at 12:45):bnjbvr created PR Review Comment:
As suggested by @julian-seward1 on zulip, do you think it'd make sense to keep it simple and stupid, and only assume a trap instruction as the embedded instruction? Only tests use a Nop4 instead of a Udf here, as far as I can tell, and it seems it would avoid a few headaches (what if the embedded instruction is a synthetic instruction, what if we compose several CondSkip within each other, potential early clobber problem buried inside this instruction...), and the use of a Box here.
If so, Julian's suggestion of
trapifmakes sense as a name, since it matches the CLIF IR instruction name, and real machine encodings on some other platforms.
Wasmtime GitHub notifications bot (Jul 02 2020 at 12:45):bnjbvr created PR Review Comment:
uber nit, but this can be something else than 8 in the current form (say, if the embedded instruction requires generates many machine instructions). Maybe something like
cbz {}, $label ; { } ; $label:?(same thing a few times below)
Wasmtime GitHub notifications bot (Jul 02 2020 at 12:45):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jul 02 2020 at 18:03):cfallin updated PR #1962 from aarch64-lowering-condbr to main:
In discussions with @bnjbvr, it came up that generating
OneWayCondBrs
with explicit, hardcoded PC-offsets as part of lowered instruction
sequences is actually unsafe, because the register allocator might
insert a spill or reload into the middle of our sequence. We were
careful about this in some cases but somehow missed that it was a
general restriction. Conceptually, all inter-instruction references
should be via labels at the VCode level; explicit offsets are only ever
known at emission time, and resolved by theMachBuffer.To allow the simple single-in, single-out local control flow that e.g.
trap checks require, without modifying the CFG (as seen by regalloc)
during lowering, this PR instead adds aCondSkippseudo-instruction
that conditionally skips a single embedded instruction. This is used to
skip around trap instructions in various places. It lowers to the same
condbr label ; trap ; label: ...sequence, but without the hardcoded
branch-target offset.<!--
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 02 2020 at 18:04):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jul 02 2020 at 18:04):cfallin created PR Review Comment:
Yes, I like the idea of keeping this better-scoped -- changed to
Inst::TrapIf. Thanks!
Wasmtime GitHub notifications bot (Jul 02 2020 at 18:05):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Jul 02 2020 at 18:05):cfallin created PR Review Comment:
Keeping this now that it is just
TrapIf, as the offset is now truly always8.
Wasmtime GitHub notifications bot (Jul 02 2020 at 21:05):cfallin merged PR #1962.
Last updated: Dec 23 2024 at 13:07 UTC