PMI diffs are failing due to assert #51728
Comments
|
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
dotnet-issue-labeler
bot
added
the
untriaged
|
This was on 120f374, which is about 12 commits behind latest. I don't see any commits that would have addressed the assert in that delta. |
jeffschwMSFT
added
the
area-CodeGen-coreclr
JulieLeeMSFT
removed
the
untriaged
|
Same as #10821. |
|
I don't think there is an easy fix. Also I don't think there is an actual GC issue here. I could be wrong on both counts. |
|
It looks like this arises from cases like having an arg The root issue appears to be that the assert only acknowledges |
|
Changing |
|
GC tracking has an inherent idea that GC refs are not created out of thin air. So IIRC the object allocator phase does the right sort of analysis to motivate potentially retyping the local, if the only defs/uses of the local are byrefs. |
@AndyAyersMS, Sorry, I might have explained myself badly. We have a That is, we have We then move continue in the next basic block with |
|
Updating the logic you added in // If we end up swapping type we may need to retype the tree:
if (retType != newType)
{
if ((retType == TYP_BYREF) && (tree->OperGet() == GT_IND))
{
// - in an RVA static if we've reinterpreted it as a byref;
assert(newType == TYP_I_IMPL);
JITDUMP("Updating type of the return GT_IND expression to TYP_BYREF\n");
inlineCandidate->gtType = TYP_BYREF;
}
else
{
// - under a call if we changed size of the argument.
GenTree* putArgType = comp->fgCheckCallArgUpdate(data->parent, inlineCandidate, retType);
if (putArgType != nullptr)
{
inlineCandidate = putArgType;
}
}
}
+ if ((retType == TYP_BYREF) && inlineCandidate->OperIsLocal())
+ {
+ unsigned lclNum = inlineCandidate->AsLclVarCommon()->GetLclNum();
+ LclVarDsc* lclVar = &comp->lvaTable[lclNum];
+
+ if (lclVar->TypeGet() == TYP_I_IMPL)
+ {
+ JITDUMP("Updating type of V%02u to TYP_BYREF\n", lclNum);
+ lclVar->lvType = TYP_BYREF;
+ }
+ }Looks to workaround the current issue as the arg gets tracked as a |
|
Updating + if ((GetEmitter()->emitInitGCrefRegs != gcInfo.gcRegGCrefSetCur) ||
+ (GetEmitter()->emitInitByrefRegs != gcInfo.gcRegByrefSetCur))
+ {
+ JITDUMP("Adding label due to changed set of live GC refs\n");
+ needLabel = true;
+ }
if (needLabel)
{
// Mark a label and update the current set of live GC refs
block->bbEmitCookie = GetEmitter()->emitAddLabel(gcInfo.gcVarPtrSetCur, gcInfo.gcRegGCrefSetCur,
gcInfo.gcRegByrefSetCur, false DEBUG_ARG(block->bbNum));
}This changes it as follows: Mapped BB01 to G_M15816_IG02
Label: IG02, GCvars=0000000000000000 {}, gcrefRegs=00000002 {rcx}, byrefRegs=00000000 {}
+ Mapped BB03 to G_M15816_IG03
+ Label: IG03, GCvars=0000000000000000 {}, gcrefRegs=00000002 {rcx}, byrefRegs=00000004 {rdx}
+ Mapped BB05 to G_M15816_IG04
+ Label: IG04, GCvars=0000000000000000 {}, gcrefRegs=00000002 {rcx}, byrefRegs=00000204 {rdx r9}
- Mapped BB06 to G_M15816_IG04
- Label: IG04, GCvars=0000000000000000 {}, gcrefRegs=00000000 {}, byrefRegs=00000000 {}
+ Mapped BB06 to G_M15816_IG06
+ Label: IG06, GCvars=0000000000000000 {}, gcrefRegs=00000000 {}, byrefRegs=00000000 {} |
|
I think this "loophole" has something to do with From what I can tell, we set it (and We don't actually emit an instruction for So when we get to BB03, the "liveness" set has changed, but its not been tracked and so inserting a new label updates |
|
So it seems like, in the process of We wouldn't be broken here in any scenario where the |
|
or more explicitly, if |
|
Thanks for drilling in. Yes, we may need to update gc info for "elided moves". In past compilers we sometimes had no-encode pseudo ops for things like this. Do the moves get elided by emitter / codegen peepholes? Or is it from LSRA preferencing? |
In this particular case, by emitter/codegen peepholes such as: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/codegenxarch.cpp#L4512-L4516 We do this fairly frequently for SIMD nodes and never through a centralized helper. You can search for Its unclear how common this is for general purpose moves, given we have some late stage peepholes and scattered calls to the emitter otherwise. It's likewise unclear if there is anything needed in LSRA preferencing to handle this. Given we have the following (and other) assert(s) trying to catch same register moves, I'd guess all code paths have this issue in some form or another: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/emitxarch.cpp#L4168-L4174 |
|
I'd have to look more closely to be sure, but what you are describing mirrors some concerns I had working on a reg-reg shuffle elimination (which we dropped in favor of better RA preferencing): dotnet/coreclr#21959 (comment) So yes even if we skip emitting an instruction we need to process the liveness update if it can impact GC. |
|
@AndyAyersMS, a quick test confirms that retaining the same register moves does fix the issue. What is the preferred way to handle this and to help prevent these issues in the future? It sounds like we want to add a new Another thought is we might be able to take advantage of the various blocks knowing their initial The priority of this, given it should only impact It is possible that most of the issues would be handled simply in |
|
@AndyAyersMS, I spent some time last night and searched through codegen/emit for all of the move elimination and tried to centralize it: https://github.com/dotnet/runtime/compare/main...tannergooding:track-moves?expand=1 There is at least one spot I've missed since |
|
When I looked into this recently I thought we might need a different kind of fix: #10821 (comment) . Not clear to me yet if what you've found supersedes this, or there is indeed some other path where we can hit this assert. Emitting GC updates for movs, while technically reasonable, is at best a bit of a crutch. Given the volume of your proposed changes I'm not sure it is a direction we should follow. Arguably we should have a similar assert for movs like we have for adds and never (or almost never, there may be one or two cases we handle) allow a mov that creates a byref. |
How would that be handled when cases like the following generate just public unsafe ref int M(int* x) {
return ref *x;
} |
|
We'd have to assume in a case like that the callers all get it right, and that treating x as a byref is ok. |
@AndyAyersMS, sorry I don't think I'm following here. We can have a
In all of these cases, provided and in these cases we end up with logic that effectively converts a pointer to a byref, such as:
Since However, when we get to codegen we have logic that basically says: if (tgtReg == srcReg)
{
// Do Nothing
}
else
{
emitMove(tgtReg, srcReg);
}Since we only do liveness updates for emitted instructions and since we opt to not emit an instruction here, we lose And so, when we get to the assert in We fail, because we dropped the liveness update. The correct fix, as far as I can see, is to not drop the liveness update in emit. There are probably several ways we could do that, one of which is to not elide in codegen, but instead to elide in emit when we try to output the instruction. |
ghost
added
in-pr
|
@tannergooding you started working on this with #52661, but that was just part 1 for a subsequent part 2 that would fix this issue (right?). Are you working on part 2? (Can I assign this to you?) |
That's the intent, yes.
I started the work here but got stuck on a few test failures and haven't had time to finish diagnosing yet. https://github.com/tannergooding/runtime/tree/byref-liveness is the current branch, and its relatively small overall. It basically just stops eliding same register moves and marks those descriptors as being zero size, allowing them to be carried through emit (without actually emitting anything) and ultimately emit the liveness update. |
ghost
added
the
in-pr
ghost
removed
the
in-pr
|
Reopening since the PR had to be reverted |
|
This assert is planned to be disabled in #55861 |
|
Well, the change that was planning to disable this assert (along with other changes) was aborted. So we maybe should just disable the assert (and #54007?) as a separate change. |
The emitter has asserts that an xarch RMW inc/dec/add/sub of a byref must have an incoming gcref/byref to be legal. This is (no longer) true due to extensive use of Span and Unsafe constructs, where we often see lclheap or other integer typed values cast to byref. Also, the emitter only updates its GC info when an instruction is generated. When one of these casts from integer to byref ends up getting the same register allocated, and its instruction is thus omitted, the emitter doesn't get the appropriate gcref update (this problem is being attempted to be solved elsewhere). For now, disable these asserts. Re-enable the tests disabled in dotnet#54207 Fixes dotnet#51728, dotnet#54007
ghost
added
the
in-pr
The emitter has asserts that an xarch RMW inc/dec/add/sub of a byref must have an incoming gcref/byref to be legal. This is (no longer) true due to extensive use of Span and Unsafe constructs, where we often see lclheap or other integer typed values cast to byref. Also, the emitter only updates its GC info when an instruction is generated. When one of these casts from integer to byref ends up getting the same register allocated, and its instruction is thus omitted, the emitter doesn't get the appropriate gcref update (this problem is being attempted to be solved elsewhere). For now, disable these asserts. Re-enable the tests disabled in #54207 Fixes #51728, #54007
ghost
removed
the
in-pr
ghost
locked as resolved and limited conversation to collaborators
jit-diff.bat diff --diff --pmi --frameworkscurrently fails for the following methods:PREPALL type# 1350 method# 20377 System.Text.Encoding::GetCharsWithFallbackPREPALL type# 1350 method# 20378 System.Text.Encoding::GetCharsWithFallbackPREPALL type# 1350 method# 20472 System.Text.Encoding::GetBytesWithFallbackPREPALL type# 1350 method# 20473 System.Text.Encoding::GetBytesWithFallbackPREPALL type# 1350 method# 20477 System.Text.Encoding::GetCharCountWithFallbackPREPALL type# 1350 method# 20478 System.Text.Encoding::GetCharCountWithFallbackIn all cases, an assert in
emitOutputRRis firing: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/emitxarch.cpp#L11738-L11739The text was updated successfully, but these errors were encountered: