Repository navigation
PMI diffs are failing due to assert #51728
Description
jit-diff.bat diff --diff --pmi --frameworks currently 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::GetCharCountWithFallback
In all cases, an assert in emitOutputRR is firing: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/emitxarch.cpp#L11738-L11739
case INS_add:
case INS_sub:
assert(id->idGCref() == GCT_BYREF);
#ifdef DEBUG
regMaskTP regMask;
regMask = genRegMask(reg1) | genRegMask(reg2);
// r1/r2 could have been a GCREF as GCREF + int=BYREF
// or BYREF+/-int=BYREF
assert(((regMask & emitThisGCrefRegs) && (ins == INS_add)) ||
((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub)));
#endif
// Mark r1 as holding a byref
emitGCregLiveUpd(GCT_BYREF, reg1, dst);
break;
Activity
tannergooding commentedon Apr 23, 2021
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.
BruceForstall commentedon Apr 23, 2021
Same as #10821.
@AndyAyersMS
AndyAyersMS commentedon Apr 23, 2021
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.
tannergooding commentedon Apr 27, 2021
It looks like this arises from cases like having an arg
byte* pOriginalBytesand then doingnew ReadOnlySpan<byte>(pOriginalBytes, originalByteCount).Slice(bytesConsumedSoFar). The arg is apointerand we correctly track it becomes abyrefwithin the scope ofBB02(ingenCodeForBBlist), but for the purposes of what is eventually asserted, we only look to trackbyrefsofvar locations, nottemporaries.The root issue appears to be that the assert only acknowledges
BYREF = GCREF + int,BYREF = BYREF + int, andBYREF = BYREF - int(or the commutative alternatives). It doesn't expectBYREF = int + int, which is what we effectively have here (byte* + int).JitDump.txt
tannergooding commentedon Apr 27, 2021
Changing
_pointer = new ByReference<T>(ref Unsafe.As<byte, T>(ref *(byte*)pointer));in Span/ROSpan to be_pointer = new ByReference<T>(ref Unsafe.AsRef<T>(pointer));causes this to repro in a lot more places (even though they should logically be "the same") 😄AndyAyersMS commentedon Apr 27, 2021
GC tracking has an inherent idea that GC refs are not created out of thin air. So
int + int -> byrefis not ok, it means one of thoseintwas really abyref, and the problem lies upstream somewhere.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.
tannergooding commentedon Apr 27, 2021
@AndyAyersMS, Sorry, I might have explained myself badly. We have a
byte*that is being stored in the byref field of a promotedSpan, so its not actually abyrefjust retyped as one. The node represented however isBYREF = BYREF + int.That is, we have
V01which isbyte*and the first thing that happens is this goes intoV40, thenV40goes intoV32, andV32goes intoV28.However, the basic block tracks no byref regs coming out of it:
We then move continue in the next basic block with
Byref regsstill being0, cuasing an assert when we hitIN0009:tannergooding commentedon Apr 27, 2021
Updating the logic you added in
fgUpdateInlineReturnExpressionPlaceHolderto do:// 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
byrefthe entire way throughtannergooding commentedon Apr 28, 2021
Updating
genCodeForBBlistto insert a new label if the set of live GC refs has changed also looks to work: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 {}14 remaining items
BruceForstall commentedon Jun 2, 2021
@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?)
tannergooding commentedon Jun 2, 2021
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.
tannergooding commentedon Jun 11, 2021
Reopening since the PR had to be reverted
BruceForstall commentedon Jul 17, 2021
This assert is planned to be disabled in #55861
BruceForstall commentedon Jul 22, 2021
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.
Disable JIT xarch asserts on gcref/byref computation
Disable JIT xarch asserts on gcref/byref computation (#56192)