Skip to content

JIT: generalize checking for valid IR after a tail call#51903

Merged
AndyAyersMS merged 1 commit intodotnet:mainfrom
AndyAyersMS:FixPostTailCallChecker
Apr 27, 2021
Merged

JIT: generalize checking for valid IR after a tail call#51903
AndyAyersMS merged 1 commit intodotnet:mainfrom
AndyAyersMS:FixPostTailCallChecker

Conversation

@AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Apr 27, 2021

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

Closes #51898.

@AndyAyersMS
JIT: generalize checking for valid IR after a tail call
b40321e 
Allow NOPs and assignments with non-overflowing casts, in addition
to the assignments we already allowed.

Closes dotnet#51898.
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 27, 2021
@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Apr 27, 2021

@EgorBo PTAL
cc @dotnet/jit-contrib

@jakobbotsch
Copy link
Member

jakobbotsch commented Apr 27, 2021

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
Copy link
Member Author

AndyAyersMS commented Apr 27, 2021

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

1 similar comment
@AndyAyersMS

This comment has been minimized.

Sign in to view
@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Apr 27, 2021

@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
Copy link
Contributor

sandreenko commented Apr 27, 2021

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
Copy link
Member Author

AndyAyersMS commented Apr 27, 2021

So yes it seems like it is a stale comment.

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

@AndyAyersMS AndyAyersMS merged commit 2d71fe2 into dotnet:main Apr 27, 2021
@AndyAyersMS AndyAyersMS deleted the FixPostTailCallChecker branch April 27, 2021 21:59
@jakobbotsch jakobbotsch mentioned this pull request Apr 27, 2021
Closed
@jakobbotsch
Copy link
Member

jakobbotsch commented Apr 27, 2021

Sure, opened #51957 for it.

@EgorBo EgorBo mentioned this pull request May 4, 2021
Merged
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Jun 8, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@EgorBo EgorBo EgorBo approved these changes

Assignees

No one assigned

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

crossgen2 determinism fails with tiered pgo

5 participants

@AndyAyersMS @jakobbotsch @sandreenko @EgorBo @karelz