-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Test failure JIT/Directed/nullabletypes/Desktop/boxunboxvaluetype_ro/boxunboxvaluetype_ro.sh #77661
Comments
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsRun: runtime-coreclr gcstress-extra 20221029.1 Failed test:
Error message:
|
Trying to repro on my MacM1, no luck so far... |
Still no luck getting a repro, either via running the test collection via xunit or running the test in isolation, and whether using runfo downloaded bits or my locally built bits, with various forms of sideload stress applied to the machine. |
Given that I can't repro, let's see if these recur over this weekend's runs. If not, I will close. |
Looks like it failed again, but on Linux/Windows. So will try reproing there instead of on OSX. |
Ok, I can readily repro on arm64 windows, and just need to run the single test case. Jit stress isolation has pinned this down to the test method itself:
so it fails with
|
Trying to further isolate with |
I guess it's my confusion on what |
Failed in Run: runtime-coreclr gcstress-extra 20221106.1 Failed tests:
Error message:
Stack trace:
|
Key stress modes are
Was going to use the inliner's
Fragment of the saved inline XML:
The offset in that last child shouldn't be 11. not -1. Inline trees of base run and log replay:
|
Managed to hack things so that the right IL offset is emitted in the XML and used during lookup. But inline reduction didn't prove all that helpful -- at least the first few top levels of the tree for So, moving on to debugging the failure. |
The bug is here: we have a copy that sets up a no-gc region, but right afterwards we refer to the register used in the copy: G_M50722_IG23: ; offs=001050H, size=0008H, bbWeight=1 PerfScore 3.00, gcrefRegs=1F700000 {x20 x21 x22 x24 x25 x26 x27 x28}, byrefRegs=0000 {}, BB33 [0106], byref, nogc
IN03ff: 001050 ldr x0, [fp, #0x60]
IN0400: 001054 str x0, [fp, #0x48]
G_M50722_IG24: ; offs=001058H, size=0054H, bbWeight=1 PerfScore 28.50, BB33 [0106], isz, extend
IN0401: 001058 ldr w1, [x0, #0x08]8] we should establish that This comes from a peephole opt where we suppress a load because the register already has the right value.
So seems like we should make sure the previous IG has the same gc properties as the current one. |
I thought we didn't do peeps across two IGs except when the current IG is an "extend" IG. And I didn't think "extend" IGs could have different GC properties. |
Proably so, but |
My prospective fix is firing in one PMI case on arm64: ;; Microsoft.CodeAnalysis.Diagnostics.AnalysisScope:ShouldInclude(Microsoft.CodeAnalysis.Diagnostic):bool:this
;; before
G_M26361_IG17: ; , nogc, extend
ldp x0, x1, [x21]
stp x0, x1, [fp, #0x48]
ldr x0, [x21, #0x10]
str x0, [fp, #0x58]
;; size=16 bbWeight=0.50 PerfScore 4.50
G_M26361_IG18: ; , isz, extend
ldr x1, [x0]
ldr x1, [x1, #0x48]
;; after
G_M26361_IG17: ; , nogc, extend
ldp x0, x1, [x21]
stp x0, x1, [fp, #0x48]
ldr x0, [x21, #0x10]
str x0, [fp, #0x58]
;; size=16 bbWeight=0.50 PerfScore 4.50
G_M26361_IG18: ; , isz, extend
ldr x0, [fp, #0x58]
; gcrRegs +[x0]
ldr x1, [x0] So we might want to consider backporting a fix. |
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.
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. Closes #77661.
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 #77661.
* JIT: fix gc hole in peephole optimizations 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 #77661. * revise per feedback Co-authored-by: Andy Ayers <[email protected]>
Run: runtime-coreclr gcstress-extra 20221029.1
Failed test:
Error message:
The text was updated successfully, but these errors were encountered: