Stream: git-wasmtime

Topic: wasmtime / PR #4856 s390x: update some regalloc metadata ...

Wasmtime GitHub notifications bot (Sep 02 2022 at 19:12):

cfallin opened PR #4856 from s390x-ra2-semantics to main:

This is a step toward ultimately removing modify-operands, which along
with removal of pinned vregs, lets us move to a completely
constraint-based and fully-SSA regalloc input and get some nice
advantages eventually.

There are still a few uses of mod operands and pinned vregs remaining,
especially around the "regpair" abstraction. Those proved to be a bit
trickier to update though, so will have to be done separately.

<!--

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 05 2022 at 13:57):

uweigand created PR review comment:

This is just a minor nit, but it would feel slightly cleaner to me to pass rn instead of rd.to_reg(). (Of course those two expression have the same value in this branch of the if.) In fact, I'm even wondering whether it wouldn't be cleanest to rename all those ri to rn -- that should make it more obious that a AluRR { op, rd, rn, rm } has identical semantic to a AluRRR { op, rd, rn, rm } if rd is tied to rn.

Wasmtime GitHub notifications bot (Sep 05 2022 at 13:57):

uweigand created PR review comment:

Now these changes I really dislike, sorry. I've been careful to ensure that the disassembly output of cranelift is always valid s390x assembler code, and could be fed right back into the assembler (e.g. to verify binary encodings).

With these changes we now get lots of output that simply isn't valid assembler -- I'd really prefer to avoid that. Now, I understand that the same printers in mod.rs are also used on the intermediate representation, and there you definitely need to see both vregs of the tied operands ...

What would you think of a solution using a helper like pretty_print_tied_regpair or so, which would show just a single (real) register name if the two operands match, but would show something like dst|src (or dst<-src or whatever) if they do not?

Wasmtime GitHub notifications bot (Sep 05 2022 at 13:57):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Sep 05 2022 at 13:57):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Sep 05 2022 at 13:57):

uweigand created PR review comment:

Again probably cleaner to use r instead of dst here.

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

uweigand submitted PR review.

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

uweigand created PR review comment:

In fact, the whole same_reg logic can probably go away then, as the assertion that the two register are actually tied is now made during emit anyway.

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

cfallin updated PR #4856 from s390x-ra2-semantics to main.

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

cfallin submitted PR review.

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

cfallin created PR review comment:

This is a great idea actually; thanks! It nicely solves the issue of showing all info when viewing pre-regalloc VCode, but showing machine code as it really is once regalloc occurs. I've made this change, and I plan to adopt it to the mod operands in the other backends as well.

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

cfallin submitted PR review.

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

cfallin created PR review comment:

Updated to use rn's value, thanks. I actually lean slightly toward keeping these fields named ri, to make it clear that they are artificial instruction fields, the "input" side of the dest reg, rather than a real rn field (this also made it easier to grep for things when updating tests just now!). But I'm happy to alter the field name as well if you feel strongly about this.

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

cfallin created PR review comment:

Done!

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

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 09 2022 at 09:55):

uweigand created PR review comment:

These two lgr look new - this is why the new code is two instructions longer than the old code. Not sure if this is something that could still be improved in regalloc, or if this is just one of those random changes ... In any case, not a big deal, I just wanted to point it out.

Wasmtime GitHub notifications bot (Sep 09 2022 at 09:55):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Sep 09 2022 at 09:55):

uweigand submitted PR review.

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

cfallin submitted PR review.

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

cfallin created PR review comment:

I briefly looked but it was nothing really obvious. It's possible that my changes in bytecodealliance/regalloc2#74 might help a bit, but I'm not sure; let's see if it reverts back once I update tests there with this merged :-)

Wasmtime GitHub notifications bot (Sep 09 2022 at 22:12):

cfallin updated PR #4856 from s390x-ra2-semantics to main.

Wasmtime GitHub notifications bot (Sep 09 2022 at 22:13):

cfallin has enabled auto merge for PR #4856.

Wasmtime GitHub notifications bot (Sep 09 2022 at 22:44):

cfallin requested elliottt for a review on PR #4856.

Wasmtime GitHub notifications bot (Sep 09 2022 at 22:44):

cfallin requested alexcrichton for a review on PR #4856.

Wasmtime GitHub notifications bot (Sep 09 2022 at 22:44):

cfallin requested fitzgen for a review on PR #4856.

Wasmtime GitHub notifications bot (Sep 09 2022 at 23:43):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 09 2022 at 23:43):

cfallin merged PR #4856.

Last updated: Nov 22 2024 at 16:03 UTC