Stream: git-wasmtime

Topic: wasmtime / PR #5427 Remove MachInst::gen_constant

Wasmtime GitHub notifications bot (Dec 13 2022 at 19:14):

uweigand opened PR #5427 from no-getconstant to main:

This addresses the problem described in issue https://github.com/bytecodealliance/wasmtime/issues/4426 that targets currently have to duplicate code to emit constants between the ISLE logic and the gen_constant callback.

The PR is split into a series of commits:

aarch64: constant generation cleanup

• Add support for MOVZ and MOVN generation via ISLE.
• Handle f32const, f64const, and nop instructions via ISLE.
• No longer call Inst::gen_constant from lower.rs.

Note that this (and future patches in the series) causes some changes in code generation where a different (but semantically equivalent) instruction is used. This is because the ISLE and Rust implementation of constant generation have already diverged somewhat. The new generated code seems preferable to me according to the Arm ISA documentation.

riscv64: constant generation cleanup

• Handle f32const and f64const instructions via ISLE.

s390x: constant generation cleanup

• Fix rule priorities for imm term.
• Only handle 32-bit stack offsets; no longer use load_constant64.

x64: constant generation cleanup

• No longer call Inst::gen_constant from lower.rs or abi.rs.

Refactor LowerBackend::lower to return InstOutput

No longer write to the per-insn output registers; instead, return an InstOutput vector of temp registers holding the outputs.

This will allow calling LowerBackend::lower multiple times for the same instruction, e.g. to rematerialize constants.

When emitting the primary copy of the instruction during lowering, writing to the per-insn registers is now done in lower_clif_block.

As a result, the ISLE lower_common routine is no longer needed. In addition, the InsnOutput type and all code related to it can be removed as well.

Refactor IsleContext to hold a LowerBackend reference

Remove the triple, flags, and isa_flags fields that are copied from LowerBackend to each IsleContext, and instead just hold a reference to LowerBackend in IsleContext.

This will allow calling LowerBackend::lower from within callbacks in src/machinst/isle.rs, e.g. to rematerialize constants.

To avoid having to pass LowerBackend references through multiple functions, eliminate the lower_insn_to_regs subroutines in those targets that still have them, and just inline into the main lower routine. This also eliminates lower_inst.rs on aarch64 and riscv64.

Replace all accesses to the removed IsleContext fields by going through the LowerBackend reference.

Remove MachInst::gen_constant

After the various cleanups in earlier patches in this series, the only remaining user of gen_constant is put_value_in_regs in Lower. This can now be removed, and instead constant rematerialization can be performed in the put_in_regs ISLE callback by simply directly calling LowerBackend::lower on the instruction defining the constant (using a different output register).

Since put_value_in_regs no longer performs rematerialization anywhere, the check for egraph mode can be removed, which in turn makes the Lower::flags member obsolete.

Care needs to be taken that other calls directly to the Lower::put_value_in_regs routine now handle the fact that no more rematerialization is performed. All such calls in target code already historically handle constants themselves. The remaining call site in the ISLE gen_call_common helper can be redirected to the ISLE put_in_regs callback.

The existing target implementations of gen_constant are then unused and can be removed. (In some target there may still be further opportunities to remove duplication between ISLE and some local Rust code - this can be left to future patches.)

CC @cfallin @jameysharp @elliottt @fitzgen

<!--

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 (Dec 13 2022 at 19:46):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 13 2022 at 19:46):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 13 2022 at 19:46):

cfallin created PR review comment:

Does this mean that we now have this rematerialization behavior unconditionally, i.e. we lost the not-egraph-mode condition? (I see that in your commit message but it's slightly unclear if it's replaced by a check anywhere else that I might be missing.) The reason for this was that the egraph-based mid-end does its own rematerialization handling, and will replicate iconsts/fconsts into different blocks as appropriate, so we want to avoid this redundant behavior. (Ultimately once egraph-based optimization is on by default we can remove this remat code here entirely.)

Wasmtime GitHub notifications bot (Dec 13 2022 at 20:05):

uweigand updated PR #5427 from no-getconstant to main.

Wasmtime GitHub notifications bot (Dec 13 2022 at 20:05):

uweigand created PR review comment:

Ah, I see, I may have misinterpreted the logic. I've added the egraphs check back here.

Wasmtime GitHub notifications bot (Dec 13 2022 at 20:05):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Dec 13 2022 at 20:32):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Dec 13 2022 at 20:32):

bjorn3 created PR review comment:

Is there any case where lower_branch is expected to return temporary registers? If not can this assertion be moved into lower_branch and the return value turned back into ()?

Wasmtime GitHub notifications bot (Dec 13 2022 at 20:38):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Dec 13 2022 at 20:38):

uweigand created PR review comment:

Well, I thought it more important to keep lower_branch just a pass-through to generated_code::constructor_lower_branch now (like the main lower routine). Ideally, the lower_branch ISLE term would have a Unit result instead of InstOutput, but that would require a few further changes (like the side_effect constructor no longer works etc.), so I didn't want to do it as part of this patch. I guess this could be done as a follow-up?

Wasmtime GitHub notifications bot (Dec 13 2022 at 20:43):

cfallin created PR review comment:

Yeah, I think that's a reasonable cleanup to do in another PR.

Wasmtime GitHub notifications bot (Dec 13 2022 at 20:43):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 13 2022 at 20:46):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 13 2022 at 21:00):

cfallin merged PR #5427.

Last updated: Dec 23 2024 at 13:07 UTC