codegen,gc-lowering: post fixup tbaa information #32321
Conversation
|
Test case? If only at the LLVM level? Can you explain what causes the bug here? The linked LLVM revisions seems to talk about forwarding an unrelocated value across a statepoint, but since we don't do that, I assume it's not quite the same issue. |
|
It's the same issue, although since our GC isn't quite as aggressive at modifying pointers at our state points, we don't need to be quite as conservative. For example, it seems likely that we can more easily keep |
There was a problem hiding this comment.
LGTM, but I still think there needs to be a better explanation somewhere of why this is necessary. I wrote this code and it took me a few minutes to understand. Maybe something like:
Certain metadata (invariant.load, tbaa_const) allows reordering of memory operations over unknown calls (including safepoint). Before GC lowering this is legal, because the optimizer sinking a load over a safepoint will automatically extend the live range of the heap object (by definition the live range of the heap object before GC lowering matches the live ranges of any reference to it). However, after GC lowering, we have fixed the live ranges of the heap objects, and thus it is no longer legal to sink such memory operations over safepoints (as the update to the object's live range won't be tracked).
Certain metadata (invariant.load, tbaa_const) allows reordering of memory operations over unknown calls (including safepoint). Before GC lowering this is legal, because the optimizer sinking a load over a safepoint will automatically extend the live range of the heap object (by definition the live range of the heap object before GC lowering matches the live ranges of any reference to it). However, after GC lowering, we have fixed the live ranges of the heap objects, and thus it is no longer legal to sink such memory operations over safepoints (as the update to the object's live range won't be tracked). fix #32215
|
OK, I've made that the commit message. I agree it's not something quite expected (as corroborated by the coreclr folks discovering a similar issue in their thinking too). I'm not even sure if this is entirely sufficient, or if we need to be even more aggressive at scrubbing attributes that lose their validity after the pass runs (like the patch above). But this at least fixed the reported issue. |
|
Im guessing this should be backported since the closed issue is marked for backport? |
Certain metadata (invariant.load, tbaa_const) allows reordering of memory operations over unknown calls (including safepoint). Before GC lowering this is legal, because the optimizer sinking a load over a safepoint will automatically extend the live range of the heap object (by definition the live range of the heap object before GC lowering matches the live ranges of any reference to it). However, after GC lowering, we have fixed the live ranges of the heap objects, and thus it is no longer legal to sink such memory operations over safepoints (as the update to the object's live range won't be tracked). fix #32215 (cherry picked from commit b2304c5)
fix #32215
unfortunately, while LLVM has a pass for exactly this purpose (RewriteStatepointsForGC::stripNonValidData),
C++ access rules prohibit us from using it and would require us to be named "coreclr"
to avoid an assertion error :/
See also https://reviews.llvm.org/D33756 for a more detailed explanation of the problem here.