JIT: generalize checking for valid IR after a tail call

[AndyAyersMS]

Allow NOPs and assignments with non-overflowing casts, in addition
to the assignments we already allowed.

Closes #51898.

[AndyAyersMS]

@EgorBo PTAL
cc @dotnet/jit-contrib

[jakobbotsch]

Is this valid in the view of this comment in the importer?

runtime/src/coreclr/jit/importer.cpp

Lines 7762 to 7768 in 9a31832

	// Note that we can not relax this condition with genActualType() as the
	// calling convention dictates that the caller of a function with a small
	// typed return value is responsible for normalizing the return val.
	if (callerRetType == calleeRetType)
	{
	return true;
	}

[AndyAyersMS]

Is this valid ...

Good question. I think perhaps that comment and check is stale.

In this case we have inlined a number of bool returning methods in tail position, leaving behind an un-inlined call that we then try to tail call:

Inlines into 0600011E [via DefaultPolicy] ILCompiler.Compilation:IsEffectivelySealed(Internal.TypeSystem.MethodDesc):bool:this
  [1 IL=0007 TR=000020 060004E4] [profitable inline guarded devirt] ILCompiler.DependencyAnalysis.ReadyToRun.DevirtualizationManager:IsEffectivelySealed(Internal.TypeSystem.MethodDesc):bool:this
    [2 IL=0016 TR=000043 0600004F] [profitable inline] ILCompiler.DevirtualizationManager:IsEffectivelySealed(Internal.TypeSystem.MethodDesc):bool:this
      [7 IL=0015 TR=000088 060004E3] [profitable inline devirt] ILCompiler.DependencyAnalysis.ReadyToRun.DevirtualizationManager:IsEffectivelySealed(Internal.TypeSystem.TypeDesc):bool:this
        [0 IL=0016 TR=000187 0600004E] [FAILED: unprofitable inline] ILCompiler.DevirtualizationManager:IsEffectivelySealed(Internal.TypeSystem.TypeDesc):bool:this

When we inline we add return value casts, per

runtime/src/coreclr/jit/importer.cpp

Lines 16991 to 16998 in 6af5918

	// Do we have to normalize?
	var_types fncRealRetType = JITtype2varType(info.compMethodInfo->args.retType);
	if ((varTypeIsSmall(op2->TypeGet()) || varTypeIsSmall(fncRealRetType)) &&
	fgCastNeeded(op2, fncRealRetType))
	{
	// Small-typed return values are normalized by the callee
	op2 = gtNewCastNode(TYP_INT, op2, false, fncRealRetType);
	}

and those casts are what we're skipping over here.

Also note in morph we tolerate a form of this where the casts wrap the call; this is just a different factorization where because of inlining we end up spreading out the cast chain over a series of assignments.

runtime/src/coreclr/jit/morph.cpp

Lines 7727 to 7768 in 6af5918

	#ifdef DEBUG
	// Tail call needs to be in one of the following IR forms
	// Either a call stmt or
	// GT_RETURN(GT_CALL(..)) or GT_RETURN(GT_CAST(GT_CALL(..)))
	// var = GT_CALL(..) or var = (GT_CAST(GT_CALL(..)))
	// GT_COMMA(GT_CALL(..), GT_NOP) or GT_COMMA(GT_CAST(GT_CALL(..)), GT_NOP)
	// In the above,
	// GT_CASTS may be nested.
	genTreeOps stmtOper = stmtExpr->gtOper;
	if (stmtOper == GT_CALL)
	{
	assert(stmtExpr == call);
	}
	else
	{
	assert(stmtOper == GT_RETURN || stmtOper == GT_ASG || stmtOper == GT_COMMA);
	GenTree* treeWithCall;
	if (stmtOper == GT_RETURN)
	{
	treeWithCall = stmtExpr->gtGetOp1();
	}
	else if (stmtOper == GT_COMMA)
	{
	// Second operation must be nop.
	assert(stmtExpr->gtGetOp2()->IsNothingNode());
	treeWithCall = stmtExpr->gtGetOp1();
	}
	else
	{
	treeWithCall = stmtExpr->gtGetOp2();
	}
	// Peel off casts
	while (treeWithCall->gtOper == GT_CAST)
	{
	assert(!treeWithCall->gtOverflow());
	treeWithCall = treeWithCall->gtGetOp1();
	}
	assert(treeWithCall == call);
	}
	#endif

[AndyAyersMS]

@dotnet/jit-contrib anyone else want to weigh in?

Do we normalize small return types in callee or caller? I vaguely recalled we changed our approach sometime back...

[sandreenko]

Do we normalize small return types in callee or caller? I vaguely recalled we changed our approach sometime back...

Do you mean if we have short Test(int v) { return (short)v; } who is cleaning the upper bytes of the return value?
The Test is responsible for clearing the upper bits for short integer types, for structs they can be left in an undefined state.

So the caller of the Test can use the register as an int knowing that the upper bits are empty:

IN0002: 000009 call     C:Test(int):short
IN0003: 00000E mov      ecx, eax
IN0004: 000010 call     System.Console:WriteLine(int)
IN0005: 000015 mov      eax, 100

[AndyAyersMS]

So yes it seems like it is a stale comment.

@jakobbotsch perhaps you can look into what happens if we remove that check?

[jakobbotsch]

Sure, opened #51957 for it.