JIT: fix gc hole in peephole optimizations #78074
Conversation
We cannot safely peephole instructions that straddle a gc enable boundary. Detecting when this might happen is a bit subtle; currently we rely on `emitForceNewIG` to be set. Add a new utility 'emitCanPeepholeLastIns` to centralize the logic that decides whether basing current emission on `emitLastIns` is safe. Closed dotnet#77661.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
@BruceForstall PTAL One diff on arm64, still running x64 locally but don't expect much there. |
|
One other thought -- if we had emitted the GC liveness update even though we didn't emit the instruction then the peephole in #77661 would have been ok. Kind of makes me think that instead of not emitting instructions we should just shrink them to zero size or something similar, But that would have some annoying aspects too (perhaps we could discard them after the gc updates are done). |
|
Per SPMI, just one arm64 method with diffs -- see #77661 (comment). |
|
/backport to release/7.0 |
|
Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3429844601 |
Are you suggesting that an instruction that gets removed by a peephole optimization would still have its GC effect applied? That seems dangerous. It seems like the peephole optimization needs to always do the right thing w.r.t. GC no matter what that might be. |
The net effect on GC needs to be the same whether we do the peephole or not. Maybe we can at least sanity check this is true somehow? Say save gc state before, then compare after states with/without peephole, they should match. Probably tricky to pull off. |
|
FYI I think #77153 is the same bug -- so this may be more widespread than I first thought. I tracked this down by doing an SPMI collect of the (intermittently failing) libraries run and then running SPMI diffs with and without the fix here. |
ghost
locked as resolved and limited conversation to collaborators
We cannot safely peephole instructions that straddle a gc enable boundary. Detecting when this might happen is a bit subtle; currently we rely on
emitForceNewIGto be set.Add a new utility
emitCannotPeepholeLastInsto centralize the logic that decides whether basing a peephole opt onemitLastInsis safe.Closes #77661.