Skip to content

Make multiple defs of one vreg possible on one instruction.#38

Merged
cfallin merged 1 commit into
bytecodealliance:mainfrom
cfallin:flexible-multi-defs
Apr 4, 2022
Merged

Make multiple defs of one vreg possible on one instruction.#38
cfallin merged 1 commit into
bytecodealliance:mainfrom
cfallin:flexible-multi-defs

Conversation

@cfallin
Copy link
Copy Markdown
Member

@cfallin cfallin commented Apr 2, 2022

Currently, if this is done, overlapping liveranges are created, and we
hit an assert that ensures our non-overlapping and
built-in-reverse-order invariants during liverange construction.

One could argue that multiple defs of a single vreg don't make a ton of
sense -- which def's value is valid after the instruction? -- but if
they all get the same alloc, then the answer is "whatever was put in
that alloc", and this is just a case of an instruction being a bit
over-eager when listing its registers.

This can arise in practice when operand lists come from combinations or
concatenations: for example, in Cranelift's s390x backend, there is a
"Loop" pseudo-instruction, and the operands of the Loop are the operands
of all the sub-instructions.

It seems more logically cohesive overall to say that one can state an
operand as many times as one likes; so this PR makes it so.

@cfallin cfallin requested a review from fitzgen April 2, 2022 05:10
@cfallin
Copy link
Copy Markdown
Member Author

cfallin commented Apr 2, 2022

As a meta-comment, I tried my best to work around requiring this change in regalloc2 at first; e.g. deduplicating at the Cranelift level, or only using RealRegs (and deduplicating those), or ... but I kept getting a big mess. I think this is cleaner; it generalizes the class of programs we can accept and makes life easier for embedders.

@Amanieu
Copy link
Copy Markdown
Contributor

Amanieu commented Apr 2, 2022

I'm not sure how I feel about this since the assert may help catch unintentional bugs in codegen. Could this be made opt-in in some way? Perhaps allow_multiple_vreg_defs on the Function trait?

Currently, if this is done, overlapping liveranges are created, and we
hit an assert that ensures our non-overlapping and
built-in-reverse-order invariants during liverange construction.

One could argue that multiple defs of a single vreg don't make a ton of
sense -- which def's value is valid after the instruction? -- but if
they all get the same alloc, then the answer is "whatever was put in
that alloc", and this is just a case of an instruction being a bit
over-eager when listing its registers.

This can arise in practice when operand lists come from combinations or
concatenations: for example, in Cranelift's s390x backend, there is a
"Loop" pseudo-instruction, and the operands of the Loop are the operands
of all the sub-instructions.

It seems more logically cohesive overall to say that one can state an
operand as many times as one likes; so this PR makes it so.
@cfallin cfallin force-pushed the flexible-multi-defs branch from 7388c60 to 4edc497 Compare April 4, 2022 06:19
@cfallin
Copy link
Copy Markdown
Member Author

cfallin commented Apr 4, 2022

Sure, yep, that's a good idea; I've added that trait method with it defaulting to false. Thanks!

Longer-term, I think it should be possible to eliminate the need for this support: I'd like to do something about our macroinstructions in Cranelift in general, and solving that would make for a cleaner input where every inst is a simple one without a variable number of args (EDIT: except calls I guess but those we can handle without this) and/or duplicated PRegs. I'll file a followup issue on the Cranelift side once we merge initial regalloc2 integration.

@cfallin cfallin merged commit 0cb0809 into bytecodealliance:main Apr 4, 2022
@cfallin cfallin deleted the flexible-multi-defs branch April 4, 2022 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants