[WIP] Remove unnecessary constant for current managed thread ID

[Copilot]

Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.

Background

In PR #129954 (#129954), CORINFO_HELP_GETCURRENTMANAGEDTHREADID on CoreCLR was turned into an IL-based DYNAMICJITHELPER bound to the managed Environment.get_CurrentManagedThreadId property (which reads the t_currentManagedThreadId [ThreadStatic]), and the Thread.CurrentThread.ManagedThreadId intrinsic was wired to convert that helper into an inline candidate (via impConvertToUserCallAndMarkForInlining) so it folds into the thread-static read.

In the PR review thread, VSadov pointed out a leftover that is now both unnecessary and potentially incorrect for runtime-async methods:

        // A constant within any method.
        case CORINFO_HELP_GETCURRENTMANAGEDTHREADID:
            vnf = VNF_ManagedThreadId;
            break;

Can that somehow cause issues in async methods? The thread ID is not a constant across suspend points.

EgorBo confirmed the fix:

I think that can be deleted now that you marked it as inline candidate (and I expect it to be always inlined) so that helper should almost never make it to the VN phase. If it doesn't inline for whatever reason it's not a big deal if it doesn't get that VN func.

The problem with the current VNF_ManagedThreadId mapping is that it gives the helper call a stable, well-known value number treated as "a constant within any method". That lets value numbering CSE multiple reads and hoist the read out of loops / treat it as loop-invariant. But the managed thread ID is not constant across an await/suspend point in a runtime-async method — after suspension the continuation may resume on a different thread, so a value numbered as a per-method constant could incorrectly be reused with a stale value. Now that the helper is virtually always inlined into a plain thread-static read before VN runs, the dedicated VN func is no longer needed and should be removed so that, in the rare case the helper survives to VN, it just gets a fresh/opaque value number like an ordinary impure helper call (no CSE, no hoisting, no cross-suspend-point reuse).

Required change (minimal and focused)

1. src/coreclr/jit/valuenum.cpp — In Compiler::fgValueNumberJitHelperMethodVNFunc, delete the dedicated case:

       // A constant within any method.
       case CORINFO_HELP_GETCURRENTMANAGEDTHREADID:
           vnf = VNF_ManagedThreadId;
           break;

   (including the // A constant within any method. comment). After removal, CORINFO_HELP_GETCURRENTMANAGEDTHREADID will no longer be mapped to a pure value-numbering function in this switch; the helper call will instead get an opaque/fresh VN through the normal helper-call handling, which is the desired behavior (no CSE / no loop hoisting / correct across async suspend points).

2. src/coreclr/jit/valuenumfuncs.h — Remove the now-unused VN func definition:

   ValueNumFuncDef(ManagedThreadId, 0, false, false)

   Only remove it after confirming (via a repo-wide search for VNF_ManagedThreadId, ManagedThreadId in the jit VNFunc/value-numbering code, and any VNFuncName/printing/tables) that nothing else references this VNFunc. If there are remaining references that would break the build, update or remove them appropriately so the JIT still builds. The expectation is that this VNFunc is only referenced by the case being deleted in step 1.

Constraints / guardrails

• Keep CORINFO_HELP_GETCURRENTMANAGEDTHREADID and the entire JIT/EE interface intact. Do NOT remove the helper from corinfo.h, CorInfoHelpFunc.cs, jithelpers.h, readytorun.h, readytorunhelpers.h, ReadyToRunConstants.cs, the AOT/R2R CorInfoImpl helper-id mappings, or JitHelper.cs. The helper, its DYNAMICJITHELPER binding to Environment.get_CurrentManagedThreadId, and the impConvertToUserCallAndMarkForInlining inline-candidate path in importercalls.cpp must all stay as-is.
• Do NOT bump JITEEVersionIdentifier in src/coreclr/inc/jiteeversionguid.h — removing an internal JIT value-numbering function does not change the JIT/EE interface surface.
• Do NOT modify HelperCallProperties::init() in src/coreclr/jit/utils.cpp (the isPure/exceptions arms for this helper). This change is specifically and only about removing the dedicated VNF_ManagedThreadId value-numbering treatment; leave the helper-properties classification unchanged.
• Keep the diff as small as possible — ideally just the two deletions above (plus any strictly necessary fixups to keep valuenumfuncs.h-driven tables building).

Validation

• Build clr+libs (release) and confirm it compiles. Note that valuenumfuncs.h is included in multiple places (e.g. s_vnfOpAttribs, VNFuncNameArr, ValidateValueNumStoreStatics), so verify those generated tables still build after removing the ManagedThreadId entry.
• Confirm there are no remaining references to VNF_ManagedThreadId anywhere in the JIT after the change.
• Sanity check that Environment.CurrentManagedThreadId and Thread.CurrentThread.ManagedThreadId still fold to the inlined thread-static read of t_currentManagedThreadId (the inline-candidate path is unchanged; this change only affects the now-essentially-dead VN path).
• In the PR description, briefly note the async-correctness motivation: a runtime-async method that reads Environment.CurrentManagedThreadId / Thread.CurrentThread.ManagedThreadId on both sides of an await must not have the second read CSE'd/hoisted to reuse the pre-suspension value, since the continuation may resume on a different thread. Removing the VNF_ManagedThreadId "constant within any method" mapping ensures that, if the helper ever survives to VN, it is not treated as a per-method constant.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: @copilot make a PR that removes CORINFO_HELP_GETCURRENTMANAGEDTHREADID

Assistant: I'll research the codebase to understand how CORINFO_HELP_GETCURRENTMANAGEDTHREADID is used before creating a PR to remove it. Let me start by verifying the repository and gathering context.

Assistant: This is a follow-up to the just-merged PR #129954. That PR rebound CORINFO_HELP_GETCURRENTMANAGEDTHREADID to the managed property, but left the helper enum entry in place. Now you want to remove the helper entirely. Let me investigate all the places the helper is referenced so the coding agent's problem statement is fully actionable.

Assistant: I have a clear picture of the existing usages. Let me look at how the CORINFO_HELP_GETCURRENTMANAGEDTHREADID is used in the JIT importer, since PR #129954 added the special handling in importercalls.cpp. I need to understand how the helper is emitted so I can plan its complete removal.

Assistant: I have a good understanding of where the helper is used. This is a significant cross-cutting JIT change in the dotnet/runtime CoreCLR codebase, touching the JIT importer, value numbering, the R2R helper table, NativeAOT/ReadyToRun compilers, and corinfo enums. This is exactly the kind of complex, multi-file task best handled by the coding agent. Let me gather a bit more context on the importer/ManagedThreadId intrinsic handling so I can write a precise problem statement.

Assistant: Let me look at how the JIT importer handles NI_System_Threading_Thread_get_ManagedThreadId and the helper call, plus the value numbering for VNF_ManagedThreadId. This is the most complex part to get right.

Assistant: The repo's default branch index may not have the merged PR changes yet. Let me look at the actual merged PR files and the current importercalls.cpp / valuenum.cpp content directly.

Assistant: I now have a full understanding of the merged PR #129954 and all the places CORINFO_HELP_GETCURRENTMANAGEDTHREADID is used. Let me check the value numbering handling and a couple more locations before creating the PR.

Assistant: Let me search for where the helper is mapped to its value number function and check the getHelperCallProperties/fgValueNumberHelperCall mapping, plus check the ECall/runtime side (FCThrow lists, vm/jithelpers.cpp registration via DYNAMIC, and any GetThreadId).

Assistant: I now have a complete understanding of the codebase. The user (VSadov, a .NET runtime team member who merged PR #129954) wants a follow-up PR that removes CORINFO_HELP_GETCURRENTMANAGEDTHREADID entirely.

After PR #129954, the helper CORINFO_HELP_GETCURRENTMANAGEDTHREADID is just an inlinable wrapper around the managed Environment.CurrentManagedThreadId property (a thread-static read). Since both Environment.CurrentManagedThreadId and the Thread.CurrentThread.ManagedThreadId intrinsic now route through this property, the helper enum can be removed and the intrinsic can directly emit a call to the managed property.

Let me verify the JIT helper enum is contiguous and check the remaining references before drafting the PR.

Assistant: I now have a comprehensive understanding. Let me look at the JIT-EE GUID file, the AOT CorInfoImpl.RyuJit.cs context, and check that there's nothing else I need to handle (like gtIsTypeHandleToRuntimeTypeHandleHelper lists or s_helperCallProperties).

Assistant: I've now gathered enough information to understand what removing CORINFO_HELP_GETCURRENTMANAGEDTHREADID would entail. This is a complex, cross-component JIT/EE interface change. Let me kick off the work.

A few important things I want to flag up front, based on what I found in the code:

• This is a JIT/EE interface change. CORINFO_HELP_GETCURRENTMANAGEDTHREADID is a member of the CorInfoHelpFunc enum in corinfo.h. Removing an enum member changes the interface, so the JITEEVersionIdentifier GUID in jiteeversionguid.h must be regenerated. There's also a parallel CorInfoHelpFunc.cs enum (used by the AOT/R2R tools) that must be kept in lock-step.
• The helper has many consumers, not just the recently-merged PR coreclr: implement Environment.CurrentManagedThreadId via ManagedThreadId.Current #129954. It's referenced in the JIT (importercalls.cpp, utils.cpp, valuenum.cpp/fgValueNumberJitHelperMethodVNFunc → VNF_ManagedThreadId), the R2R format (readytorun.h, readytorunhelpers.h, ReadyToRunConstants.cs, the R2R/RyuJit/Reflection AOT compilers, JitHelper.cs), and jithelpers.h/corelib.h. After PR coreclr: implement Environment.CurrentManagedThreadId via ManagedThreadId.Current #129954, on CoreCLR the helper is already just an IL-based DYNAMICJITHELPER bound to Environment.get_CurrentManagedThreadId, and the Thread.CurrentThread.ManagedThreadId intrinsic in importercalls.cpp lowers to it.

I'll have the coding agent investigate the full removal (replacing the helper with a direct managed call to Environment.CurrentManagedThreadId / ManagedThreadId.Current, including the R2R version-compat implications) and produce a PR.

User: We probably still need the helper, just remove the treatment for it in terms of it hoisting.

Assistant: Got it — that's a much narrower change than a full helper removal. Let me look at exactly where CORINFO_HELP_GETCURRENTMANAGEDTHREADID gets special "can be hoisted / optimized away" treatment so I can scope this precisely.

Assistant: Now I have a complete understanding. The user wants the helper kept (so the JIT/EE interface stays intact and R2R version-compat doesn't break), and only the special "hoisting" treatment of the helper removed.

Let me look at exactly what makes the helper hoistable — the isPure flag in utils.cpp (HelperCallProperties::init) is what allows the JIT to hoist/CSE/optimize-away the helper call. Let me also check the value numbering treatment.

In valuenum.cpp, CORINFO_HELP_GETCURRENTMANAGEDTHREADID → VNF_ManagedThreadId, which is defined in valuenumfuncs.h as ValueNumFuncDef(ManagedThreadId, 0, false, false). This gives it a stable value number, which combined with isPure=true in utils.cpp enables hoisting/CSE.

I now have enough to create the PR.

Assistant: I've outlined the change and queued it up for the coding agent — it just needs your confirmation to start, so we're waiting on your go-ahead before any work begins.

Here's what I'm asking it to do, based on your guidance to keep the helper but drop the hoisting treatment:

Keep CORINFO_HELP_GETCURRENTMANAGEDTHREADID and the entire JIT/EE interface (the CorInfoHelpFunc enums, jithelpers.h DYNAMICJITHELPER binding to Environment.get_CurrentManagedThreadId, R2R mappings, etc.) intact — no GUID bump.

Remove the special "this helper is pure → hoist/CSE it" treatment, in the spots I found it:

1. HelperCallProperties::init() in src/coreclr/jit/utils.cpp — the dedicated case that sets isPure = true (this is the flag that authorizes loop-hoisting and CSE). It becomes non-pure (but can stay no-throw / no-GC so it isn't pessimized to the default mutatesHeap = true arm).
2. The second near-identical switch in the same file (the gtNodeHasSideEffects-style value-properties switch) that also marks it isPure = true/exceptions = None — addressed consistently so it isn't treated as pure in either place.
3. Value numbering — fgValueNumberJitHelperMethodVNFunc maps it to VNF_ManagedThreadId (declared in valuenumfuncs.h). A stable well-known VN is itself what enables CSE and loop-invariant hoisting, so I've asked the agent to evaluate removing that dedicated VNFunc (giving the call a fresh/opaque VN like an ordinary impure helper) and to remove the now-unused ManagedThreadId VNFunc def if nothing else references it.

One thing worth your call: dropping isPure alone stops hoisting/CSE via the side-effect path, but the dedicated VNF_ManagedThreadId value number can still let VN-based CSE coalesce two reads. That's why I included step 3 — I think the cleanest result is to also drop the VNFunc so two reads get distinct VNs. If you'd rather keep VNF_ManagedThreadId for some reason, let me know and I'll have it only flip isPure and confirm VN doesn't re-enable CSE.

Want me to proceed as specified, or adjust the VN handling first?

User: For additional info:: #129954 (comment)

Assistant: This is the key context. The comment thread (especially comment 4837403573 from EgorBo, and VSadov's 4837306511) confirms exactly the concern and the fix:

• VSadov noticed the leftover case CORINFO_HELP_GETCURRENTMANAGEDTHREADID: vnf = VNF_ManagedThreadId; in fgValueNumberJitHelperMethodVNFunc, and worries the "constant within any method" treatment is wrong for async methods — the thread ID is not constant across await/suspend points, so a hoisted/CSE'd VNF_ManagedThreadId could reuse a stale value after the continuation resumes on a different thread.
• EgorBo's answer: now that the helper is an inline candidate (and basically always inlines), it should almost never reach the VN phase, so the VNF_ManagedThreadId mapping can just be deleted; if it ever fails to inline, getting a fresh/opaque VN is fine.

So this nails down the change precisely — it's narrower than my earlier draft. The right fix is just to delete the dedicated VN treatment (and the now-unused VNF_ManagedThreadId), not to touch the isPure helper-properties at all. Let me cancel the prior queued task and re-scope it.

Updated plan, grounded in the thread:

• Delete the case CORINFO_HELP_GETCURRENTMANAGEDTHREADID: vnf = VNF_ManagedThreadId; break; arm (with its // A constant within any method. comment) from fgValueNumberJitHelperMethodVNFunc in valuenum.cpp. The helper then falls through to the normal path and gets a fresh/opaque VN, so it's no longer treated as a per-method constant that can be CSE'd/hoisted across suspend points.
• Remove the now-unused ValueNumFuncDef(ManagedThreadId, 0, false, false) from valuenumfuncs.h (and confirm via search there are no other VNF_ManagedThreadId/VNFunc::ManagedThreadId references before removing it).
• Do NOT touch HelperCallProperties::init() / the isPure arms in utils.cpp — the thread didn't ask for that, and the motivating bug is specifically the async-correctness of the VN "constant" treatment.
• Keep the helper, the JIT/EE interface, and the inline-candidate path intact; no GUID bump.

Let me re-issue the task with this corrected, minimal scope.

Assistant: That comment thread clears things up — thanks. I've revised the task accordingly and it's queued, waiting on your confirmation to start.

Here's the key correction from the discussion (VSadov + EgorBo in #issuecomment-4837403573):

• The thing to remove is only the value-numbering treatment — the leftover case CORINFO_HELP_GETCURRENTMANAGEDTHREADID: vnf = VNF_ManagedThreadId; in fgValueNumberJitHelperMethodVNFunc (plus the now-unused ValueNumFuncDef(ManagedThreadId, ...) in valuenumfuncs.h).
• Why: that VN func marks the read as "a constant within any method," which is wrong for runtime-async — the managed thread ID isn't constant across an await/suspend point (the continuation can resume on a different thread), so CSE/hoisting could reuse a stale value. EgorBo confirmed it's safe to delete because the helper is now (almost) always inlined to the thread-static read before VN runs; if it ever survives, a fresh/opaque VN is the correct, harmless outcome.

So I've dropped the earlier plan to touch HelperCallProperties::init() (the isPure arms) in utils.cpp — that wasn't what the maintainers asked for, and EgorBo's guidance is specifically about the VN func. The task now is just the two deletions, keeping the helper, the JIT/EE interface, and the inline-candidate path fully intact (no GUID bump).

Want me to go ahead with that scoped change?

[VSadov]

seems like a dupe of #130006