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

Bugfix: no hole in liveranges for pinned vreg move src. #60

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Jun 26, 2022

Right now, pinned vregs are a way of naming real registers (a
compatibility shim of sorts for Cranelift's RealRegs) and can be used
as sources and dests of moves. When the input program does so, regalloc2
converts these into "ghost" uses and defs on the other vreg of the move
(dest or source, respectively) with a fixed-register constraint. So
move v128, v0 where v0 is pinned to p0 turns into a "ghost def"
on v128 with constraint fixed p0.

There is some fancy manipulation of liveranges to make this all work
while properly recording where the preg's value must be preserved.
Unfortunately, there was an off-by-one in the location of the move and
transition of live-ranges which interacts poorly with the "implicit
live-in" of pinned vregs at function start. As a result, a function body
that starts like:

    move v128, v0
    def v9000
    move v129, v1

might allocate p1 (to which v1 is pinned) for v9000. This clobbers
the original value.

Fortunately this only impacts the implicit live-in, and Cranelift's use
of regalloc2 is such that it will always copy all values out of pinned
vregs (creating ghost defs) without intervening defs, except in the
case of sret ("structure return") arguments. If a program does not use
sret arguments (and the cranelift-wasm frontend does not), then this
bug should not be reachable.

Long-term, we really need to kill pinned vregs with fire (#3); the
special cases that arise from these, and from special handling of moves,
are too much incidental complexity. All of this can go away once
Cranelift migrates all fixed-register cases to operand constraints
instead. That will be a happy day.

Thanks to @bjorn3 for finding and reporting this issue!

Right now, pinned vregs are a way of naming real registers (a
compatibility shim of sorts for Cranelift's `RealReg`s) and can be used
as sources and dests of moves. When the input program does so, regalloc2
converts these into "ghost" uses and defs on the other vreg of the move
(dest or source, respectively) with a fixed-register constraint. So
`move v128, v0` where `v0` is pinned to `p0` turns into a "ghost def"
on `v128` with constraint `fixed p0`.

There is some fancy manipulation of liveranges to make this all work
while properly recording where the preg's value must be preserved.
Unfortunately, there was an off-by-one in the location of the move and
transition of live-ranges which interacts poorly with the "implicit
live-in" of pinned vregs at function start. As a result, a function body
that starts like:

```
    move v128, v0
    def v9000
    move v129, v1
```

might allocate `p1` (to which `v1` is pinned) for `v9000`. This clobbers
the original value.

Fortunately this only impacts the implicit live-in, and Cranelift's use
of regalloc2 is such that it will always copy all values out of pinned
vregs (creating ghost defs) without intervening defs, *except* in the
case of `sret` ("structure return") arguments. If a program does not use
`sret` arguments (and the `cranelift-wasm` frontend does not), then this
bug should not be reachable.

Long-term, we really need to kill pinned vregs with fire (bytecodealliance#3); the
special cases that arise from these, and from special handling of moves,
are too much incidental complexity. All of this can go away once
Cranelift migrates all fixed-register cases to operand constraints
instead. That will be a happy day.

Thanks to @bjorn3 for finding and reporting this issue!
@cfallin cfallin requested a review from fitzgen June 26, 2022 21:31
@cfallin
Copy link
Member Author

cfallin commented Jun 26, 2022

My plan for this re: other PRs: we will want to land this first, and do a 0.2.3 release of regalloc2, and then do a point-release of Cranelift that bumps the RA2 dep. Then we'll want to land #58 and #59 and version-bump to 0.3, since #58 changes the API slightly.

@cfallin cfallin merged commit 68aa615 into bytecodealliance:main Jun 27, 2022
@cfallin cfallin deleted the fix-pinned-vreg-mov-src branch June 27, 2022 18:21
@cfallin cfallin mentioned this pull request Jun 27, 2022
cfallin added a commit to cfallin/wasmtime that referenced this pull request Jun 27, 2022
cfallin added a commit to cfallin/wasmtime that referenced this pull request Jun 27, 2022
alexcrichton pushed a commit to bytecodealliance/wasmtime that referenced this pull request Jun 27, 2022
…loc2#60. (#4333)

* Upgrade to regalloc2 v0.2.3 to get bugfix from bytecodealliance/regalloc2#60.

* Update RELEASES.md.

* Update two compile tests based on slightly shifting regalloc output.

* Add dividing line in RELEASES.md as per format.
alexcrichton pushed a commit to bytecodealliance/wasmtime that referenced this pull request Jun 27, 2022
…loc2#60. (#4335)

* Upgrade to regalloc2 v0.2.3 to get bugfix from bytecodealliance/regalloc2#60.

* Update RELEASES.md.

* Update two compile tests based on slightly shifting regalloc output.
afonso360 pushed a commit to afonso360/wasmtime that referenced this pull request Jun 30, 2022
…loc2#60. (bytecodealliance#4335)

* Upgrade to regalloc2 v0.2.3 to get bugfix from bytecodealliance/regalloc2#60.

* Update RELEASES.md.

* Update two compile tests based on slightly shifting regalloc output.
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.

2 participants