Wasmtime GitHub notifications bot (Sep 09 2022 at 21:59): Wasmtime GitHub notifications bot (Sep 09 2022 at 21:59):cfallin requested elliottt for a review on PR #4892.
Wasmtime GitHub notifications bot (Sep 09 2022 at 21:59):cfallin requested fitzgen for a review on PR #4892.
Wasmtime GitHub notifications bot (Sep 09 2022 at 21:59):cfallin opened PR #4892 from abi-reg-constraints to main:
This PR updates the shared ABI code and backends to use register-operand constraints rather than explicit pinned-vreg moves for register arguments and return values.
The s390x backend was not updated, because it has its own implementation of ABI code. Ideally we could converge back to the code shared by x64 and aarch64 (which didn't exist when s390x ported calls to ISLE, so the current situation is underestandable, to be clear!). I'll leave this for future work.
This PR exposed several places where regalloc2 needed to be a bit more flexible with constraints; it requires regalloc2#74 to be merged and pulled in. Right now CI will fail because
Cargo.tomlpoints to a local checkout; I'll update this after
the regalloc2 changes are in.<!--
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 (Sep 09 2022 at 21:59):cfallin requested bnjbvr for a review on PR #4892.
Wasmtime GitHub notifications bot (Sep 14 2022 at 16:09):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Sep 14 2022 at 16:09):fitzgen created PR review comment:
- Can we type def this small vec since it is used all over the place?
- This change is doubling the size of these small vecs since each element is two regs instead of one now, and I wonder if it makes sense to cut the inline capacity in half? Or at least remeasure a little to see where the sweet spot is now?
Wasmtime GitHub notifications bot (Sep 14 2022 at 16:09):fitzgen created PR review comment:
Let's debug assert that this isn't a call the probestack libcall here, since we rely on probestacks being inserted after regalloc and use that fact to justify avoiding listing their uses as an optimization. This way, if we ever start probestacking earlier, we will hit this assertion and be reminded to fix up that code.
Wasmtime GitHub notifications bot (Sep 14 2022 at 16:09):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Sep 14 2022 at 16:09):fitzgen created PR review comment:
Why 4? Is that the max that this will ever use? If so, then it should probably be an
arrayvecinstead. If not, I think we can go larger since this is just a one-off vector in a stack frame, not something where we have to worry too much about the size of the smallvec because it is inside a bunch of heap allocated structs.
Wasmtime GitHub notifications bot (Sep 14 2022 at 16:09):fitzgen created PR review comment:
Should probably update the docs for this trait method. We shouldn't clobber caller-saved registers anymore right? Instead we should use the given temps if needed, correct?
Wasmtime GitHub notifications bot (Sep 14 2022 at 16:09):fitzgen created PR review comment:
nitpick: Line wrap is a bit funky here.
Wasmtime GitHub notifications bot (Sep 14 2022 at 16:09):fitzgen created PR review comment:
We could allow taking the signatures out of the context temporarily, so that we could immutably borrow them while mutably borrowing the lowering context. This would avoid needing to pre-allocate temps up front.
Wasmtime GitHub notifications bot (Sep 14 2022 at 16:09):fitzgen created PR review comment:
Similarly:
&ABIArgSlot::Reg { extension: ir::ArgumentExtension::None, ty, .. } => ty.is_ref() as _, &ABIArgSlot::Reg { .. } => 1,
Wasmtime GitHub notifications bot (Sep 14 2022 at 16:09):fitzgen created PR review comment:
Could code golf this down a little bit, if you wanted:
&ABIArgSlot::Stack { extension: ir::ArgumentExtension::None, .. } => 0, &ABIArgSlot::Stack { .. } => 1,
Wasmtime GitHub notifications bot (Sep 14 2022 at 16:09):fitzgen created PR review comment:
Kinda strange to have two different kinds of comments here? Should
#just be;?
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:14):cfallin updated PR #4892 from abi-reg-constraints to main.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:32):cfallin updated PR #4892 from abi-reg-constraints to main.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:32):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:32):cfallin created PR review comment:
;is actually not a comment in the generated assembly format, though -- the punctuation above breaks down as:
- initial column
;isfilecheck's comment, and serves here to give theprecise-outputexpected text- the
;s in line 45 separate additional instructions (ldr, thenb, then adatapseudoinstruction)- the
#on line 46 introduces an actual comment that gives more info about the callso the
; ... # ...is a comment-within-a-comment, if that makes sense...
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:32):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:32):cfallin created PR review comment:
Fixed, thanks!
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:32):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:32):cfallin created PR review comment:
True, though imho that is probably best done as a follow-up refactor...
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:33):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:33):cfallin created PR review comment:
Good point, updated to 16; it's bounded only by the number of args so it could be larger.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:34):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:34):cfallin created PR review comment:
Took almost this, just a bit different -- I kept the
extension != Noneas a guard-ifon the match arm, as it reads more clearly to me that way, semantically (the case we want to handle is "has extension", not "has no extension", so the subpattern match doesn't work as well).
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:34):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:34):cfallin created PR review comment:
Updated!
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:34):cfallin created PR review comment:
Added!
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:34):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:35):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:35):cfallin created PR review comment:
Added typedefs! I kept the size at
8as IMHO it's important to avoid heap allocs for most functions here, and when I've looked before, there are a good number with more than four args. Once we have a stats framework this would be a good thing to rexamine though, I agree.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:39):cfallin updated PR #4892 from abi-reg-constraints to main.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:44):cfallin updated PR #4892 from abi-reg-constraints to main.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:49):cfallin has marked PR #4892 as ready for review.
Wasmtime GitHub notifications bot (Sep 20 2022 at 23:49):cfallin has enabled auto merge for PR #4892.
Wasmtime GitHub notifications bot (Sep 21 2022 at 00:14):cfallin has disabled auto merge for PR #4892.
Wasmtime GitHub notifications bot (Sep 21 2022 at 00:29):cfallin updated PR #4892 from abi-reg-constraints to main.
Wasmtime GitHub notifications bot (Sep 21 2022 at 00:44):cfallin has enabled auto merge for PR #4892.
Wasmtime GitHub notifications bot (Sep 21 2022 at 01:17):cfallin merged PR #4892.
Last updated: Dec 23 2024 at 12:05 UTC