System.Linq.Expressions CompileDeepTree_NoStackOverflowFast fails with Stack Overflow

[SingleAccretion]

Recent runs that observed the failure: first, second. Test in question.

To diagnose the cause, I used the following snippet:

var flags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static | BindingFlags.Instance;
var t = new Thread(x => RuntimeHelpers.PrepareMethod(typeof(Program).GetMethod("Problem", flags).MethodHandle), X * 1024);
t.Start();

class Program
{
    // Pseudo syntax, I used InlineIL.Fody
    static int Problem()
    {
        ldc.i4.0
    
        // Repeated 100 times
        ldc.i4.1
        add
    
        ret
    }
}

Release runtime + Release Jit were tested (since that's what the CI was using), with tiered compilation off.

git bisect showed the following:
- Commits before d435388 only needeed X of 65 to succeed.
- Commits after and including d435388 needed X of 129 and above to succeed, matching the timing of the failures in CI (this test succeeded on 21st of May).

This test has a history of failing on Debug Jits (indeed, an attribute pointing to just that) - see #21374. But now, apparently, it needs 129 KB of stack even in Release. I am not sure what is the proper fix for this.

cc @AndyAyersMS

[AndyAyersMS]

@jakobbotsch want to take this one?

[jakobbotsch]

The issue is that the native PGO data we are passing after d435388 causes MSVC to use more stack space in fgMorphSmpOp. Before that commit the stack frame for fgMorphSmpOp needs 0x2A0 bytes, while after it needs 0x340 bytes. As #21374 points out, the stack overflow has fgMorphSmpOp and fgMorphTree calling each other recursively, once for each GT_ADD we end up with (which is limited, see below). fgMorphTree uses 0xF0 bytes of stack in both builds, so in the failing we need 100*(0x340+0xF0) = 105 KB of stack plus what's needed for the rest of the frames.

The importer already limits the depth of trees it can create:

runtime/src/coreclr/jit/importer.cpp

Lines 11234 to 11252 in 44f050a

	/* We need to restrict the max tree depth as many of the Compiler
	functions are recursive. We do this by spilling the stack */
	if (verCurrentState.esStackDepth)
	{
	/* Has it been a while since we last saw a non-empty stack (which
	guarantees that the tree depth isnt accumulating. */
	if ((opcodeOffs - lastSpillOffs) > MAX_TREE_SIZE && impCanSpillNow(prevOpcode))
	{
	impSpillStackEnsure();
	lastSpillOffs = opcodeOffs;
	}
	}
	else
	{
	lastSpillOffs = opcodeOffs;
	impBoxTempInUse = false; // nothing on the stack, box temp OK to use again
	}

We could decrease this limit, though that seems unnecessary. The original point of this test was to have a fast inner loop test corresponding to the slower outer loop one above it. However, in #21374 this new test was also moved to outer loop, so it does not seem like there is much point in having it anymore.