Stream: git-wasmtime

Topic: wasmtime / PR #4858 ABI: implement register arguments wit...

Wasmtime GitHub notifications bot (Sep 02 2022 at 21:24):

cfallin requested fitzgen for a review on PR #4858.

Wasmtime GitHub notifications bot (Sep 02 2022 at 21:24):

cfallin requested elliottt for a review on PR #4858.

Wasmtime GitHub notifications bot (Sep 02 2022 at 21:24):

cfallin opened PR #4858 from abi-reg-constraints to main:

Currently, Cranelift's ABI code emits a sequence of moves from physical
registers into vregs at the top of the function body, one for every
register-carried argument.

For a number of reasons, we want to move to operand constraints instead,
and remove the use of explicitly-named "pinned vregs"; this allows for
better regalloc in theory, as it removes the need to "reverse-engineer"
the sequence of moves.

This PR alters the ABI code so that it generates a single "args"
pseudo-instruction as the first instruction in the function body. This
pseudo-inst defs all register arguments, and constrains them to the
appropriate registers at the def-point. Subsequently the regalloc can
move them wherever it needs to.

Some care was taken not to have this pseudo-inst show up in
post-regalloc disassemblies, but the change did cause a general regalloc
"shift" in many tests, so the precise-output updates are a bit noisy.
Sorry about that!

A subsequent PR will handle the other half of the ABI code, namely, the
callsite case, with a similar preg-to-constraint conversion.

<!--

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 02 2022 at 21:26):

cfallin updated PR #4858 from abi-reg-constraints to main.

Wasmtime GitHub notifications bot (Sep 07 2022 at 18:48):

jameysharp updated PR #4858 from abi-reg-constraints to main.

Wasmtime GitHub notifications bot (Sep 07 2022 at 18:53):

cfallin updated PR #4858 from abi-reg-constraints to main.

Wasmtime GitHub notifications bot (Sep 07 2022 at 19:15):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Sep 07 2022 at 19:15):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Sep 07 2022 at 19:15):

jameysharp created PR review comment:

If you move the vectors outside ArgInfo, making it Vec<ArgInfo> or similar, then BoxArgInfo could be defined as Box<[ArgInfo]> instead of boxing the vector. Since this would be a fat pointer, the box needs space for two usize values instead of one, but I'm guessing that wouldn't make any of the backends' Inst representations grow. I assume when you decided to box this that you were really trying to avoid sticking two Vecs in the instructions, which would need _six_ usize values.

Wasmtime GitHub notifications bot (Sep 07 2022 at 19:15):

jameysharp created PR review comment:

Every user of ArgInfo currently has to zip together defs and pregs. I try to avoid that where possible because it means there's an invariant that the two vectors have the same length, which has to be maintained across distant parts of the codebase. I'd suggest instead either using a vector of pairs, or if you want to name the elements of the pair to make it clear which is which, then have this struct represent only one constraint and keep a Vec<ArgInfo> around instead.

pub struct ArgInfo {
    /// vreg defined at the top of the function with
    /// register-argument values.
    pub def: Writable<Reg>,
    /// preg that constrains that vreg to ABI-defined locations.
    pub preg: Reg,
}

Wasmtime GitHub notifications bot (Sep 07 2022 at 20:34):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 07 2022 at 20:34):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 07 2022 at 20:34):

fitzgen created PR review comment:

This seems semi fragile. Is there a reason that we need an instruction for this and can't just constrain the collector directly at the top of VCodeBuilder::collect_operands and count_operands (ideally factoring out a run_collector_on_function helper that is used by both)? This would avoid having a magic instruction that has to always be at the top of the function and remove one extra Inst variant we would have to match on everywhere else.

Wasmtime GitHub notifications bot (Sep 07 2022 at 21:24):

cfallin created PR review comment:

Unfortunately, the whole allocator is built around operands living on instructions; there aren't any free-floating constraints that can exist separately. The alternative would be to inject the operands artificially as early-def operands on the first instruction in the body, but that seems worse to me.

FWIW we do the same thing in the other direction on return instructions already. (I guess that one is a little more tightly coupled because the return also actually does the control transfer.)

Another alternative could be to build a notion of "input constraints" into RA2, but the cleanest way to implement that IMHO would be to inject an artificial instruction in RA2's frontend, rather than have a separate kind of constraint, so it doesn't improve much in global complexity.

I may be missing an option here though, if you had something else in mind?

Wasmtime GitHub notifications bot (Sep 07 2022 at 21:24):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 07 2022 at 21:26):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Sep 07 2022 at 21:26):

elliottt created PR review comment:

Why hide the synthetic Args instructions in the disassembly?

Wasmtime GitHub notifications bot (Sep 07 2022 at 21:29):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Sep 07 2022 at 21:29):

elliottt created PR review comment:

            // The code below must come first. Finish the current "IR inst"

Wasmtime GitHub notifications bot (Sep 07 2022 at 21:30):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 07 2022 at 21:30):

cfallin created PR review comment:

I guess it's somewhat subjective, but in the post-regalloc code it seemed like unnecessary clutter as it would always indicate tautologies (args rdi=rdi, rsi=rsi, rcx=rcx, rdx=rdx, ...). Happy to remove this special case if others disagree though :-) (To be clear, it still remains in the VCode debug print either way; the want_disasm mechanism is separate)

Wasmtime GitHub notifications bot (Sep 07 2022 at 21:32):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Sep 07 2022 at 21:32):

elliottt created PR review comment:

What about cases where the args were passed on the stack? Would those end up being useful to see?

Wasmtime GitHub notifications bot (Sep 07 2022 at 21:37):

elliottt created PR review comment:

I really like the suggestion to have the args be a vector of pairs :+1:

Wasmtime GitHub notifications bot (Sep 07 2022 at 21:37):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Sep 07 2022 at 22:11):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 07 2022 at 22:11):

cfallin created PR review comment:

Ah, those are actually separately visible as loads, since args only handles registers. I suppose in the future if/when RA2 has a notion of "custom stackslot locations" we can give vregs constraints that directly indiciate where to load from, then we don't need that distinction; at that time we can do something different here.

Wasmtime GitHub notifications bot (Sep 07 2022 at 23:16):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Sep 07 2022 at 23:17):

elliottt created PR review comment:

That sounds good to me, thanks for the clarification!

Wasmtime GitHub notifications bot (Sep 08 2022 at 06:28):

cfallin updated PR #4858 from abi-reg-constraints to main.

Wasmtime GitHub notifications bot (Sep 08 2022 at 06:28):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 08 2022 at 06:28):

cfallin created PR review comment:

Fixed!

Wasmtime GitHub notifications bot (Sep 08 2022 at 06:29):

cfallin created PR review comment:

Done; this is much cleaner now, thanks!

Wasmtime GitHub notifications bot (Sep 08 2022 at 06:29):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 08 2022 at 06:30):

cfallin created PR review comment:

Updated! (Actually as a Vec<ArgPair> for now as the extra usize doesn't make a difference in the overall inst enum size and it keeps things a bit simpler otherwise)

And indeed, the original out-of-lining of the data was because otherwise the Inst types all became larger; but no longer.

Wasmtime GitHub notifications bot (Sep 08 2022 at 06:30):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 08 2022 at 18:37):

jameysharp created PR review comment:

Looks like auto-updating the file tests deleted comments like this one.

Wasmtime GitHub notifications bot (Sep 08 2022 at 18:37):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Sep 08 2022 at 18:37):

jameysharp created PR review comment:

I'd like to suggest this change too. I'd rather use Option::inspect but it's not stable yet.

            insts.into_iter().next().map(|inst| {
                trace!(
                    "gen_retval_area_setup: inst {:?}; ptr reg is {:?}",
                    inst,
                    self.ret_area_ptr.unwrap().to_reg()
                );
                inst
            })

Wasmtime GitHub notifications bot (Sep 08 2022 at 18:37):

jameysharp created PR review comment:

I think we want to keep this comment too.

Wasmtime GitHub notifications bot (Sep 08 2022 at 18:37):

jameysharp created PR review comment:

I guess as long as I'm asking for more things... something like this? It wasn't obvious to me what the "default" value here meant.

            // The `args` instruction below must come first. Finish the
            // current "IR inst" with a default source location and continue the scan backward.
            self.finish_ir_inst(Default::default());

Wasmtime GitHub notifications bot (Sep 08 2022 at 18:37):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Sep 08 2022 at 18:37):

jameysharp created PR review comment:

Here's another deleted comment.

Wasmtime GitHub notifications bot (Sep 08 2022 at 23:04):

cfallin updated PR #4858 from abi-reg-constraints to main.

Wasmtime GitHub notifications bot (Sep 08 2022 at 23:04):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 08 2022 at 23:04):

cfallin created PR review comment:

Updated, thanks!

Wasmtime GitHub notifications bot (Sep 08 2022 at 23:04):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 08 2022 at 23:04):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Sep 08 2022 at 23:05):

cfallin created PR review comment:

Oops, I'm not sure why the updater removed that... restored (and I went through the diff chunk-by-chunk to make sure the below and others were not missed either).

Wasmtime GitHub notifications bot (Sep 08 2022 at 23:05):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 08 2022 at 23:06):

cfallin requested jameysharp for a review on PR #4858.

Wasmtime GitHub notifications bot (Sep 09 2022 at 00:29):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Sep 09 2022 at 01:03):

cfallin merged PR #4858.

Last updated: Oct 23 2024 at 20:03 UTC