Skip to content

Commit

Permalink
Disable JIT xarch asserts on gcref/byref computation (#56192)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
BruceForstall authored Jul 23, 2021
1 parent aa0cf93 commit d148c34
Show file tree
Hide file tree
Showing 9 changed files with 23 additions and 14 deletions.
29 changes: 23 additions & 6 deletions src/coreclr/jit/emitxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11914,10 +11914,12 @@ BYTE* emitter::emitOutputR(BYTE* dst, instrDesc* id)
#endif
if (id->idGCref())
{
// The reg must currently be holding either a gcref or a byref
// and the instruction must be inc or dec
assert(((emitThisGCrefRegs | emitThisByrefRegs) & regMask) &&
(ins == INS_inc || ins == INS_dec || ins == INS_inc_l || ins == INS_dec_l));
assert(ins == INS_inc || ins == INS_dec || ins == INS_inc_l || ins == INS_dec_l);
// We would like to assert that the reg must currently be holding either a gcref or a byref.
// However, we can see cases where a LCLHEAP generates a non-gcref value into a register,
// and the first instruction we generate after the LCLHEAP is an `inc` that is typed as
// byref. We'll properly create the byref gcinfo when this happens.
// assert((emitThisGCrefRegs | emitThisByrefRegs) & regMask);
assert(id->idGCref() == GCT_BYREF);
// Mark it as holding a GCT_BYREF
emitGCregLiveUpd(GCT_BYREF, id->idReg1(), dst);
Expand Down Expand Up @@ -12222,15 +12224,30 @@ BYTE* emitter::emitOutputRR(BYTE* dst, instrDesc* id)
case INS_sub:
assert(id->idGCref() == GCT_BYREF);

#if 0
#ifdef DEBUG
// Due to elided register moves, we can't have the following assert.
// For example, consider:
// t85 = LCL_VAR byref V01 arg1 rdx (last use) REG rdx
// /--* t85 byref
// * STORE_LCL_VAR byref V40 tmp31 rdx REG rdx
// Here, V01 is type `long` on entry, then is stored as a byref. But because
// the register allocator assigned the same register, no instruction was
// generated, and we only (currently) make gcref/byref changes in emitter GC info
// when an instruction is generated. We still generate correct GC info, as this
// instruction, if writing a GC ref even through reading a long, will go live here.
// These situations typically occur due to unsafe casting, such as with Span<T>.

regMaskTP regMask;
regMask = genRegMask(reg1) | genRegMask(reg2);

// r1/r2 could have been a GCREF as GCREF + int=BYREF
// or BYREF+/-int=BYREF
// or BYREF+/-int=BYREF
assert(((regMask & emitThisGCrefRegs) && (ins == INS_add)) ||
((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub)));
#endif
#endif // DEBUG
#endif // 0

// Mark r1 as holding a byref
emitGCregLiveUpd(GCT_BYREF, reg1, dst);
break;
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.IO.Hashing/tests/Crc32Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase)
InstanceAppendAllocateAndResetDriver(testCase);
}

[SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)]
[Theory]
[MemberData(nameof(TestCases))]
public void InstanceMultiAppendGetCurrentHash(TestCase testCase)
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.IO.Hashing/tests/Crc64Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase)
InstanceAppendAllocateAndResetDriver(testCase);
}

[SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)]
[Theory]
[MemberData(nameof(TestCases))]
public void InstanceMultiAppendGetCurrentHash(TestCase testCase)
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.IO.Hashing/tests/XxHash32Tests.007.cs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase)
InstanceAppendAllocateAndResetDriver(testCase);
}

[SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)]
[Theory]
[MemberData(nameof(TestCases))]
public void InstanceMultiAppendGetCurrentHash(TestCase testCase)
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.IO.Hashing/tests/XxHash32Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase)
InstanceAppendAllocateAndResetDriver(testCase);
}

[SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)]
[Theory]
[MemberData(nameof(TestCases))]
public void InstanceMultiAppendGetCurrentHash(TestCase testCase)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase)
InstanceAppendAllocateAndResetDriver(testCase);
}

[SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)]
[Theory]
[MemberData(nameof(TestCases))]
public void InstanceMultiAppendGetCurrentHash(TestCase testCase)
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.IO.Hashing/tests/XxHash64Tests.007.cs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase)
InstanceAppendAllocateAndResetDriver(testCase);
}

[SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)]
[Theory]
[MemberData(nameof(TestCases))]
public void InstanceMultiAppendGetCurrentHash(TestCase testCase)
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.IO.Hashing/tests/XxHash64Tests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase)
InstanceAppendAllocateAndResetDriver(testCase);
}

[SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)]
[Theory]
[MemberData(nameof(TestCases))]
public void InstanceMultiAppendGetCurrentHash(TestCase testCase)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ public void InstanceAppendAllocateAndReset(TestCase testCase)
InstanceAppendAllocateAndResetDriver(testCase);
}

[SkipOnCoreClr("https://github.com/dotnet/runtime/issues/54007", RuntimeConfiguration.Checked)]
[Theory]
[MemberData(nameof(TestCases))]
public void InstanceMultiAppendGetCurrentHash(TestCase testCase)
Expand Down

0 comments on commit d148c34

Please sign in to comment.