Skip to content

JIT: don't stress-tailcall named intrinsics (#122479) #128336

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AndyAyersMS wants to merge 1 commit into
base: main
Choose a base branch
from
+91 −32
Open

JIT: don't stress-tailcall named intrinsics (#122479) #128336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,8 +323,6 @@ internal void SetWaitSleepJoinState()
GC.KeepAlive(this);
}

// Temporary workaround for https://github.com/dotnet/runtime/issues/122479
[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)]
internal void ClearWaitSleepJoinState()
{
// This method is called when the thread is no longer in a wait, sleep, or join state.
Expand Down
74 changes: 44 additions & 30 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9151,44 +9151,58 @@ void Compiler::impImportBlockCode(BasicBlock* block)
bool hasTailPrefix = (prefixFlags & PREFIX_TAILCALL_EXPLICIT);
if (newBBcreatedForTailcallStress && !hasTailPrefix)
{
// Do a more detailed evaluation of legality
const bool passedConstraintCheck =
checkTailCallConstraint(opcode, &resolvedToken,
constraintCall ? &constrainedResolvedToken : nullptr);

// Avoid setting compHasBackwardsJump = true via tail call stress if the method cannot have
// patchpoints.
//
const bool mayHavePatchpoints = opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) &&
(JitConfig.TC_OnStackReplacement() > 0) &&
compCanHavePatchpoints();
if (passedConstraintCheck && (mayHavePatchpoints || compHasBackwardJump))
// Don't stress-tailcall named intrinsics: many of them are imported as
// non-CALL IR nodes (e.g. GC.KeepAlive -> GT_KEEPALIVE), which would
// leave a BBJ_RETURN block that doesn't end in a CALL/RETURN and
// confuse later phases (see
// https://github.com/dotnet/runtime/issues/122479). Suppress both the
Copy link
Copy Markdown
Member

@tannergooding tannergooding May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also matches hardware and JIT intrinsics, correct?

I know its not exactly related since this is covering stress mode, but we have some other "pessimizations" around intrinsics and tail calls such as https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/importercalls.cpp#L10212-L10225 where we may refuse to import it as non-CALL IR.

I wonder if for cases that are known to expand to trivial IR, like KEEPALIVE or most HWIntrinsics, if its worth just blanket avoiding tail calls altogether or if we need to centrally handle tail. call intrinsic to ensure we never try to expand them (if its required to respect the tail call)

Copy link
Copy Markdown
Member Author

@AndyAyersMS AndyAyersMS May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall we have some vague notion of centralizing the tail call logic somewhere, though I can't remember now if it is to move it all earlier (maybe as a post-importer phase) or move it all later, into morph. Either way we'd have resolved intrinsics into closer-to-final IR before making decisions about tail calls.

// explicit and the implicit tailcall promotion in that case.
if ((callInfo.methodFlags & CORINFO_FLG_INTRINSIC) != 0)
{
// Now check with the runtime
CORINFO_METHOD_HANDLE declaredCalleeHnd = callInfo.hMethod;
bool isVirtual = (callInfo.kind == CORINFO_VIRTUALCALL_STUB) ||
(callInfo.kind == CORINFO_VIRTUALCALL_VTABLE);
CORINFO_METHOD_HANDLE exactCalleeHnd = isVirtual ? nullptr : declaredCalleeHnd;
if (info.compCompHnd->canTailCall(info.compMethodHnd, declaredCalleeHnd, exactCalleeHnd,
hasTailPrefix)) // Is it legal to do tailcall?
JITDUMP(" (Tailcall stress: skipping intrinsic)");
passedStressModeValidation = false;
}
else
{
// Do a more detailed evaluation of legality
const bool passedConstraintCheck =
checkTailCallConstraint(opcode, &resolvedToken,
constraintCall ? &constrainedResolvedToken : nullptr);

// Avoid setting compHasBackwardsJump = true via tail call stress if the method cannot have
// patchpoints.
//
const bool mayHavePatchpoints = opts.jitFlags->IsSet(JitFlags::JIT_FLAG_TIER0) &&
(JitConfig.TC_OnStackReplacement() > 0) &&
compCanHavePatchpoints();
if (passedConstraintCheck && (mayHavePatchpoints || compHasBackwardJump))
{
// Stress the tailcall.
JITDUMP(" (Tailcall stress: prefixFlags |= PREFIX_TAILCALL_EXPLICIT)");
prefixFlags |= PREFIX_TAILCALL_EXPLICIT | PREFIX_TAILCALL_STRESS;
// Now check with the runtime
CORINFO_METHOD_HANDLE declaredCalleeHnd = callInfo.hMethod;
bool isVirtual = (callInfo.kind == CORINFO_VIRTUALCALL_STUB) ||
(callInfo.kind == CORINFO_VIRTUALCALL_VTABLE);
CORINFO_METHOD_HANDLE exactCalleeHnd = isVirtual ? nullptr : declaredCalleeHnd;
if (info.compCompHnd->canTailCall(info.compMethodHnd, declaredCalleeHnd, exactCalleeHnd,
hasTailPrefix)) // Is it legal to do tailcall?
{
// Stress the tailcall.
JITDUMP(" (Tailcall stress: prefixFlags |= PREFIX_TAILCALL_EXPLICIT)");
prefixFlags |= PREFIX_TAILCALL_EXPLICIT | PREFIX_TAILCALL_STRESS;
}
else
{
// Runtime disallows this tail call
JITDUMP(" (Tailcall stress: runtime preventing tailcall)");
passedStressModeValidation = false;
}
}
else
{
// Runtime disallows this tail call
JITDUMP(" (Tailcall stress: runtime preventing tailcall)");
// Constraints disallow this tail call
JITDUMP(" (Tailcall stress: constraint check failed)");
passedStressModeValidation = false;
}
}
else
{
// Constraints disallow this tail call
JITDUMP(" (Tailcall stress: constraint check failed)");
passedStressModeValidation = false;
}
}
}
#endif
Expand Down
39 changes: 39 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_122479/Runtime_122479.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// Under STRESS_TAILCALL the JIT importer would promote a "call+ret" pattern
// to an explicit (or implicit) tailcall before discovering that the callee
// is a named intrinsic (e.g. GC.KeepAlive, which is imported as a
// GT_KEEPALIVE node, not a CALL). When combined with a [SuppressGCTransition]
// P/Invoke earlier in the same block, the resulting BBJ_RETURN ended with a
// non-CALL/non-RETURN statement and tripped an assert in fgInsertStmtNearEnd
// during "Insert GC Polls".
//
// Run under any jitstress / jitstress_tiered leg to validate.

using System;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Threading;
using Xunit;

public class Runtime_122479
{
[MethodImpl(MethodImplOptions.NoInlining)]
private static void ReproPattern()
{
// Thread.Sleep -> Thread.ClearWaitSleepJoinState which has the
// exact "SuppressGCTransition QCall; GC.KeepAlive(this); ret" shape
// that originally triggered the assert.
Thread.Sleep(0);
}

[Fact]
public static void TestEntryPoint()
{
for (int i = 0; i < 100; i++)
{
ReproPattern();
}
}
}
8 changes: 8 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_122479/Runtime_122479.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>
Loading