Conversation
|
Azure Pipelines: 16 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
|
@copilot remove changes to StructWithGC_Zeroing tests, no tests needed. |
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
|
@jakobbotsch @AndyAyersMS @dotnet/jit-contrib so this PR removes hasExplicitInit entirely (with the flag) - see PR changes with "ignore whitespaces" mode. Diffs - not that bad for non-tests collections, but still a regression. @jakobbotsch pointed out that the current change is not correct - it assumes at the SSA liveness stage we know exactly what parts of code are no-gc areas which is not correct, so presumably there might be a scenario when we have a GC local, it's not zeroed yet and we hit an unexpected gc safe point (e.g. fully interruptible part) - it will crash. Should we take it as is given it's a correctness fix? I prototyped a fix that does the same analysis at the codegen phase that recovers the regressions, but it was a bit too complex for this PR IMO. |
|
It seems that regression happen moslty in small methods where the existing opt managed to avoid any zeroing in prolog, e.g. nuint Test(ref int value)
{
fixed (int* p = &value)
return (nuint)p & 1;
}Main: ; Method WriteBarrierBenchmarks:Test(byref):nuint (FullOpts)
G_M49291_IG01:
push rax
G_M49291_IG02:
mov bword ptr [rsp], rcx
mov rax, rcx
and rax, 1
G_M49291_IG03:
add rsp, 8
ret
; Total bytes of code: 17PR: ; Method WriteBarrierBenchmarks:Test(byref):nuint (FullOpts)
G_M49291_IG01:
push rax
xor eax, eax
mov qword ptr [rsp], rax
G_M49291_IG02:
mov bword ptr [rsp], rcx
mov rax, rcx
and rax, 1
G_M49291_IG03:
add rsp, 8
ret
; Total bytes of code: 23(in the real world, it's likely inlined to somewhere where we already zero locals with SIMD anyway) |
|
I'm a bit worried about the potential impact of this change. Wondering if we should consider something more surgical. Is this a regression in .NET 11? |
The actual bug can be fixed easily in ifConversion by checking the flag. but the assumption about no-gc between prolog and a store in SSA phase cannot be fixed surgically I believe, it needs to be moved to a later phase to be correct. I assume it's been like that for a few releases. |
|
I don't think I said that it wasn't correct, just that SSA's liveness is a very early point to be trying to reason about GC safepoints. I do not know of other correctness issues but I wouldn't be surprised if others exist. We run a lot of optimizations after SSA's liveness, but not that much code motion, so that's probably how we get by. #132326 was another issue around this. I do think we should run this particular optimization much later, probably as part of LSRA or right before it. I also wouldn't be surprised if this optimization is quite beneficial with full dataflow instead of being limited to initial basic blocks and bailing on any |
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/da0dccf9-8520-43ca-9c90-bfdee6c39f9a Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.