JIT: breaking IGs for perf score leads to check-release GC info diffs

[AndyAyersMS]

As part of root-causing #39023, I was running check-release diffs via SPMI, and saw a large number of GC info diffs.

Tracked this down to this particular bit of code:

runtime/src/coreclr/src/jit/codegenlinear.cpp

Lines 347 to 367 in 24f63ec

	#if defined(DEBUG) || defined(LATE_DISASM)
	// We also want to start a new Instruction group by calling emitAddLabel below,
	// when we need accurate bbWeights for this block in the emitter. We force this
	// whenever our previous block was a BBJ_COND and it has a different weight than us.
	//
	// Note: We need to have set compCurBB before calling emitAddLabel
	//
	if ((block->bbPrev != nullptr) && (block->bbPrev->bbJumpKind == BBJ_COND) &&
	(block->bbWeight != block->bbPrev->bbWeight))
	{
	needLabel = true;
	}
	#endif // DEBUG || LATE_DISASM
	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);
	}

We shouldn't form full IG boundaries differently in check and release. It's tolerable to form different "extended" IGs, though.

cc @briansull @BruceForstall @dotnet/jit-contrib

[AndyAyersMS]

Based on some cursory analysis, I don't think the different GC info is wrong, though I'm not 100% sure. The upshot seems to be that in checked builds we terminate GC lifetimes earlier than we do in release.

[AndyAyersMS]

Sample diff jit gc encoder trace from the problematic method in #39023 (red is chk, green is rel):

@@ -46,13 +46,13 @@ Register slot id for reg r11 = 41.
 Register slot id for reg r11 (byref) = 42.
 Register slot id for reg r12 = 43.
 Set state of slot 0 at instr offset 0x58 to Live.
-Set state of slot 0 at instr offset 0xe8 to Dead.
+Set state of slot 0 at instr offset 0xfe to Dead.
 Set state of slot 1 at instr offset 0x6d to Live.
 Set state of slot 1 at instr offset 0xb8 to Dead.
 Set state of slot 2 at instr offset 0x75 to Live.
 Set state of slot 2 at instr offset 0xb8 to Dead.
 Set state of slot 3 at instr offset 0xe2 to Live.
-Set state of slot 3 at instr offset 0xe8 to Dead.
+Set state of slot 3 at instr offset 0xfe to Dead.
 Set state of slot 3 at instr offset 0x13f to Live.
 Set state of slot 3 at instr offset 0x1f5 to Dead.
 Set state of slot 4 at instr offset 0x162 to Live.
@@ -64,9 +64,9 @@ Set state of slot 6 at instr offset 0x1bc to Dead.
 Set state of slot 7 at instr offset 0x1d9 to Live.
 Set state of slot 7 at instr offset 0x1f5 to Dead.
 Set state of slot 3 at instr offset 0x236 to Live.
-Set state of slot 3 at instr offset 0x6be to Dead.
+Set state of slot 3 at instr offset 0x6ec to Dead.
 Set state of slot 7 at instr offset 0x236 to Live.
-Set state of slot 7 at instr offset 0x6be to Dead.
+Set state of slot 7 at instr offset 0x6ec to Dead.
 Set state of slot 8 at instr offset 0x272 to Live.
 Set state of slot 8 at instr offset 0x2b7 to Dead.
 Set state of slot 9 at instr offset 0x27a to Live.
@@ -90,11 +90,11 @@ Set state of slot 17 at instr offset 0x5fe to Dead.
 Set state of slot 18 at instr offset 0x487 to Live.
 Set state of slot 18 at instr offset 0x48b to Dead.
 Set state of slot 18 at instr offset 0x497 to Live.
-Set state of slot 18 at instr offset 0x4ea to Dead.
+Set state of slot 18 at instr offset 0x4f9 to Dead.
 Set state of slot 19 at instr offset 0x4bf to Live.
 Set state of slot 19 at instr offset 0x542 to Dead.
 Set state of slot 20 at instr offset 0x4d1 to Live.
-Set state of slot 20 at instr offset 0x4ea to Dead.
+Set state of slot 20 at instr offset 0x4f9 to Dead.
 Set state of slot 21 at instr offset 0x4d6 to Live.
 Set state of slot 21 at instr offset 0x51c to Dead.
 Set state of slot 22 at instr offset 0x4e0 to Live.

[AndyAyersMS]

This could also lead to codegen diffs as the emitter peepholes for x64 don't look back across IG boundaries. So it's possible we'd do a bit less peepholing on x64 chk than ret. I haven't found an example of this.

If we go the route of having the code above produce extended IGs then we still may have chk-release diffs as the x64 peepholes don't look back across extended IG boundaries. So arguably we should fix that too.

[BruceForstall]

I don't see any harm in making the highlighted code run on all configurations. I don't think we should create "extended" IGs for this purpose: nobody should ever do anything different due to extended IGs; the peepholes that behave differently in the presence of extended IGs should be fixed (I thought there was already a proposal to do so, based on doing "the right thing" for arm64 peeps).

[AndyAyersMS]

Thinking some more, this issue won't cause any new chk-rel codegen diffs; these extra IGs in chk happen only if the pred is a BBJ_COND none of our peepholes are effected as the previous instruction will be some kind of branch.