Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow fixed-early-def and fixed-late-use constraints #168

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

Amanieu
Copy link
Contributor

@Amanieu Amanieu commented Nov 19, 2023

These are useful to model fixed registers which should not be reused for other uses/defs. These were disallowed in #54, but this was too conservative.

Fundamentally, the only situation where a preg can be used in multiple fixed constraints within a single instruction is with an early-use and a late-def. Anything else is a user error because the live ranges will overlap.

As such this PR relaxes the operand rewrite from #54 to only apply in this specific situation. Fixed-late-use and fixed-early-def operands are left unchanged.

Copy link
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain the use-case for a late-use? I'm a little unclear about how that would be used, and also am not sure if we need to still worry about this comment with the changes in this pr:

                                // See note in fuzzing/func.rs: we
                                // can't allow this, because there
                                // would be no way to move a value
                                // into place for a late use *after*
                                // the early point (i.e. in the middle
                                // of the instruction).

Specifically, what guarantees that we'd never try to generate a move in the middle of an instruction to satisfy a late-use? Currently we unconditionally use the CodeRange::from program point when inserting moves, so if the liverange containing a late-use got trimmed to only include that use, what prevents us from trying to insert the move at the late position?

src/fuzzing/func.rs Outdated Show resolved Hide resolved
@Amanieu
Copy link
Contributor Author

Amanieu commented Dec 6, 2023

I personally don't have a use case for fixed-late-use, I only needed fixed-early-def in my code. I just allowed fixed-late-use for symmetry.

Specifically, what guarantees that we'd never try to generate a move in the middle of an instruction to satisfy a late-use? Currently we unconditionally use the CodeRange::from program point when inserting moves, so if the liverange containing a late-use got trimmed to only include that use, what prevents us from trying to insert the move at the late position?

Live range trimming/splitting will only split live ranges on instruction boundaries, never in the middle of an instruction. Since the value of a late use must necessarily come from a previous instruction, a move will simply be inserted before the instruction.

These are useful to model fixed registers which should not be reused for
other uses/defs. These were disallowed in bytecodealliance#54, but this was too
conservative.

Fundamentally, the only situation where a preg can be used in multiple
fixed constraints within a single instruction is with an early-use and a
late-def. Anything else is a user error because the live ranges will
overlap.

As such this PR relaxes the operand rewrite from bytecodealliance#54 to only apply in
this specific situation. Fixed-late-use and fixed-early-def operands are
left unchanged.
@elliottt
Copy link
Member

elliottt commented Dec 6, 2023

Live range trimming/splitting will only split live ranges on instruction boundaries, never in the middle of an instruction. Since the value of a late use must necessarily come from a previous instruction, a move will simply be inserted before the instruction.

Ah right, thanks!

Separately, it sounds like @afonso360 has a use for fixed-late-use constraints in the riscv64 backend in cranelift.

@afonso360
Copy link
Contributor

afonso360 commented Dec 6, 2023

I was looking for the PR where I first needed this, and it looks like it was bytecodealliance/wasmtime#6568 but weirdly, I added a fixed-late-use operand, and it worked out fine? It's still in there.

Here's some of the discussion regarding this: bytecodealliance/wasmtime#6568 (comment)

Now that I'm looking at this again I'm not sure if a fixed-late-use operand is the same as a fixed-late use constraint?

In any case I'm going to run the SIMD testsuite with this PR since it sounds like these changes would be triggered with the existing code.


As an explanation of what is going on in that PR, it introduces the vslideup instruction, which has some special constraints, namely:

  • It modifies the destination register (vd); and
  • None of the input registers (vs2 or vm) must be the same register as the destination register (vd)
  • The mask register (vm) is optional, but when provided is fixed to the v0 register

@elliottt
Copy link
Member

elliottt commented Dec 6, 2023

It looks like the instruction is only generated with an (unmasked) argument for mask, which will not cause the fixed-late-use to be generated. I think that explains why we're not seeing the assertion failure for that case.

@afonso360
Copy link
Contributor

afonso360 commented Dec 6, 2023

That makes sense. I checked the rest of the backend, and we currently only use masks with the vmerge instruction, which does not request a fixed-late-use operand on the mask.

Copy link
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this seems good, and we've got a real use in cranelift that will benefitf from this. Thanks!

What do you think about adding reg_fixed_early_def and reg_fixed_late_use functions to the Operand struct to simplify construction of these cases?

@elliottt elliottt merged commit ea86b87 into bytecodealliance:main Dec 6, 2023
6 checks passed
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.

None yet

3 participants