Skip to content

Rework and simplify initial versioning and tiering rules#125243

Open
davidwrighton wants to merge 5 commits intodotnet:mainfrom
davidwrighton:rework_and_simplify_initial_versioning_rules
Open

Rework and simplify initial versioning and tiering rules#125243
davidwrighton wants to merge 5 commits intodotnet:mainfrom
davidwrighton:rework_and_simplify_initial_versioning_rules

Conversation

@davidwrighton
Copy link
Member

  • Remove CallCountingInfo::Disabled stage and associated methods (DisableCallCounting, IsCallCountingEnabled, CreateWithCallCountingDisabled)
  • Move initial optimization tier determination from per-method JIT-time logic to a config-time decision (TieredCompilation_DefaultTier) computed once during EEConfig::sync()
  • Store the default code version's optimization tier directly on MethodDescCodeData, replacing the indirect approach of disabling call counting to signal optimized tier
  • Remove WasTieringDisabledBeforeJitting flag from PrepareCodeConfig and MethodDescVersioningState
  • Simplify GetJitFlags by removing the special default-version fast path and using the unified optimization tier switch for all code versions
  • Add OptimizationTierUnknown sentinel to distinguish uninitialized state
  • Simplify FinalizeOptimizationTierForTier0Load to handle R2R loading based on the stored optimization tier

- Remove CallCountingInfo::Disabled stage and associated methods
  (DisableCallCounting, IsCallCountingEnabled, CreateWithCallCountingDisabled)
- Move initial optimization tier determination from per-method JIT-time
  logic to a config-time decision (TieredCompilation_DefaultTier) computed
  once during EEConfig::sync()
- Store the default code version's optimization tier directly on
  MethodDescCodeData, replacing the indirect approach of disabling call
  counting to signal optimized tier
- Remove WasTieringDisabledBeforeJitting flag from PrepareCodeConfig and
  MethodDescVersioningState
- Simplify GetJitFlags by removing the special default-version fast path
  and using the unified optimization tier switch for all code versions
- Add OptimizationTierUnknown sentinel to distinguish uninitialized state
- Simplify FinalizeOptimizationTierForTier0Load to handle R2R loading
  based on the stored optimization tier

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors CoreCLR tiered compilation’s initial tier selection and tiering signaling by moving “default tier” decisions into EEConfig::sync(), removing the call-counting “disabled” signaling path, and storing the default version tier on MethodDesc code data.

Changes:

  • Compute and expose a single TieredCompilation_DefaultTier value in EEConfig, and use it for initial tier selection.
  • Add OptimizationTierUnknown and a per-MethodDesc stored optimization tier, and route default-version tier reads/writes through it.
  • Remove the call-counting “Disabled” stage and related APIs/flags, simplifying tier-related logic in the JIT flag path and prestub tier finalization.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/coreclr/vm/tieredcompilation.cpp Switch initial tier selection to use EEConfig’s computed default tier.
src/coreclr/vm/prestub.cpp Remove “disable call counting” signaling; revise tier finalization for R2R load based on stored tier.
src/coreclr/vm/method.hpp Add stored optimization tier to MethodDescCodeData; remove WasTieringDisabledBeforeJitting plumbing.
src/coreclr/vm/method.cpp Initialize and implement MethodDesc optimization tier storage helpers.
src/coreclr/vm/eeconfig.h Add TieredCompilation_DefaultTier() accessor and config backing field.
src/coreclr/vm/eeconfig.cpp Compute tieredCompilation_DefaultTier during EEConfig::sync().
src/coreclr/vm/codeversion.h Add OptimizationTierUnknown sentinel value.
src/coreclr/vm/codeversion.cpp Use stored MethodDesc tier when available; set it for default versions via MethodDesc.
src/coreclr/vm/callcounting.h Remove CallCountingInfo::Disabled stage and disabled-construction helpers.
src/coreclr/vm/callcounting.cpp Remove disabled-stage construction/guards and remove disabled-callcounting APIs’ implementations.

IfFailRet(EnsureCodeDataExists(NULL));

_ASSERTE(m_codeData != NULL);
if (InterlockedExchangeT(&m_codeData->OptimizationTier, tier) != NativeCodeVersion::OptimizationTierUnknown)
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

SetMethodDescOptimizationTier() uses InterlockedExchangeT, which overwrites any previously-set OptimizationTier even when returning S_FALSE. This breaks the intended “set once from Unknown” semantics and can race with other threads, potentially flipping a method’s tier unexpectedly. Use InterlockedCompareExchangeT to only store the tier when the current value is OptimizationTierUnknown (and return S_FALSE without modifying otherwise).

Suggested change
if (InterlockedExchangeT(&m_codeData->OptimizationTier, tier) != NativeCodeVersion::OptimizationTierUnknown)
if (InterlockedCompareExchangeT(&m_codeData->OptimizationTier, tier, NativeCodeVersion::OptimizationTierUnknown) != NativeCodeVersion::OptimizationTierUnknown)

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

I agree with copilot here. This is an indiscriminate exchange. Why don't we want InterlockedCompareExchange?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm... the latest change does a simple set, but there is some merit to not wanting to march over stuff willy nilly. I'll think about that tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking into this, I've concluded that the current logic, which simply sets the value is correct. We could in theory add asserts that its behaving as expected, but just doing the set unconditionally is acceptable.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 5, 2026 23:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Comment on lines 1033 to 1036
NativeCodeVersion nativeCodeVersion = config->GetCodeVersion();
if (nativeCodeVersion.IsDefaultVersion() && !config->WasTieringDisabledBeforeJitting())
{
MethodDesc *methodDesc = nativeCodeVersion.GetMethodDesc();
if (!methodDesc->IsEligibleForTieredCompilation())
{
_ASSERTE(nativeCodeVersion.GetOptimizationTier() == NativeCodeVersion::OptimizationTierOptimized);
return flags;
}

_ASSERT(!methodDesc->RequestedAggressiveOptimization());

if (g_pConfig->TieredCompilation_QuickJit())
{
NativeCodeVersion::OptimizationTier currentTier = nativeCodeVersion.GetOptimizationTier();
if (currentTier == NativeCodeVersion::OptimizationTier::OptimizationTier0Instrumented)
{
flags.Set(CORJIT_FLAGS::CORJIT_FLAG_BBINSTR);
flags.Set(CORJIT_FLAGS::CORJIT_FLAG_TIER0);
return flags;
}

if (currentTier == NativeCodeVersion::OptimizationTier::OptimizationTier1Instrumented)
{
flags.Set(CORJIT_FLAGS::CORJIT_FLAG_BBINSTR);
flags.Set(CORJIT_FLAGS::CORJIT_FLAG_TIER1);
return flags;
}

_ASSERTE(!nativeCodeVersion.IsFinalTier());
flags.Set(CORJIT_FLAGS::CORJIT_FLAG_TIER0);
if (g_pConfig->TieredPGO() && g_pConfig->TieredPGO_InstrumentOnlyHotCode())
{
// If we plan to only instrument hot code we have to make an exception
// for cold methods with loops so if those self promote to OSR they need
// some profile to optimize, so here we allow JIT to enable instrumentation
// if current method has loops and is eligible for OSR.
flags.Set(CORJIT_FLAGS::CORJIT_FLAG_BBINSTR_IF_LOOPS);
}
return flags;
}

methodDesc->GetLoaderAllocator()->GetCallCountingManager()->DisableCallCounting(nativeCodeVersion);
nativeCodeVersion.SetOptimizationTier(NativeCodeVersion::OptimizationTierOptimized);
return flags;
}

switch (nativeCodeVersion.GetOptimizationTier())
{
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

With the default-version fast path removed, GetJitFlags now relies entirely on nativeCodeVersion.GetOptimizationTier() and the switch below. Some switch cases mutate the tier (e.g., tier0 -> optimized when QuickJit is off), but this PR also removes the old mechanism that disabled call counting to indicate “start optimized and never tier”. Please ensure the publishing/call-counting paths (notably CallCountingManager::SetCodeEntryPoint, which is invoked from CodeVersionManager::PublishNativeCodeVersion) correctly bypass call counting for optimized default versions; otherwise it can hit assertions that assume default versions are not final tiers.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@davidwrighton davidwrighton Mar 6, 2026

Choose a reason for hiding this comment

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

My analysis indicates this was a bug, but I've fixed in the next revision.

Comment on lines 364 to 369
NativeCodeVersion codeVersion = pConfig->GetCodeVersion();
if (codeVersion.IsDefaultVersion())
{
pConfig->GetMethodDesc()->GetLoaderAllocator()->GetCallCountingManager()->DisableCallCounting(codeVersion);
_ASSERTE(codeVersion.IsFinalTier());
}
else if (!codeVersion.IsFinalTier())
if (!codeVersion.IsFinalTier())
{
codeVersion.SetOptimizationTier(NativeCodeVersion::OptimizationTierOptimized);
}
pConfig->SetWasTieringDisabledBeforeJitting();
shouldTier = false;
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

When tiering is disabled here (preemptive caller GC mode / UnmanagedCallersOnly), the code may set the default code version’s tier to OptimizationTierOptimized while the method remains eligible for tiered compilation. Since CodeVersionManager::PublishNativeCodeVersion still calls CallCountingManager::SetCodeEntryPoint for published code, make sure the call counting logic correctly treats optimized default versions as non-call-counted/final (the old implementation relied on DisableCallCounting). Otherwise this can trigger assertions in SetCodeEntryPoint that assume default versions are not final tiers.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch copilot, this was a bug in the PR.

….com:davidwrighton/runtime into rework_and_simplify_initial_versioning_rules
Comment on lines +797 to +798
// 1 - OptimizationTier0 as we don't want to instrument the initial version (will only instrument hot Tier0)
// 2 - OptimizationTier0Instrumented - instrument all ILOnly code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 1 - OptimizationTier0 as we don't want to instrument the initial version (will only instrument hot Tier0)
// 2 - OptimizationTier0Instrumented - instrument all ILOnly code
// - OptimizationTier0 as we don't want to instrument the initial version (will only instrument hot Tier0)
// - OptimizationTier0Instrumented - instrument all ILOnly code

I assume the ordinals aren't real values and this is really just a numbered list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I have some vague memories that during the experiments around this stuff it was actually a more flexible policy, but it never has had flexibility in the shipped product.

return S_OK;
}

HRESULT MethodDesc::SetMethodDescOptimizationTier(NativeCodeVersion::OptimizationTier tier)
Copy link
Member

Choose a reason for hiding this comment

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

How is this handled by MethodDesc::Reset(), does the reset logic also need to be updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

EnC methods are not eligible for tiering, so that doesn't apply, and dynamic methods aren't supported either, which are the only use cases for MethodDesc::Reset See CodeVersionManager::IsMethodSupported for details.

IfFailRet(EnsureCodeDataExists(NULL));

_ASSERTE(m_codeData != NULL);
if (InterlockedExchangeT(&m_codeData->OptimizationTier, tier) != NativeCodeVersion::OptimizationTierUnknown)
Copy link
Member

Choose a reason for hiding this comment

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

I agree with copilot here. This is an indiscriminate exchange. Why don't we want InterlockedCompareExchange?

- Remove redundant IsDefaultVersion() guard before IsFinalTier() check
  in CallCountingManager::SetCodeEntryPoint
- Query optimization tier from CodeVersion instead of MethodDesc in
  FinalizeOptimizationTierForTier0Load
- Clean up comment formatting in EEConfig::sync

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 6, 2026 20:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

}
}
#else // !FEATURE_TIERED_COMPILATION
tieredCompilation_DefaultTier = NativeCodeVersion::OptimizationTierOptimized;
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

In the !FEATURE_TIERED_COMPILATION branch, tieredCompilation_DefaultTier (a DWORD) is assigned an enum value without an explicit cast, while other assignments in this method cast to DWORD. Using a consistent explicit cast here helps avoid compiler warnings (and matches the nearby code style).

Suggested change
tieredCompilation_DefaultTier = NativeCodeVersion::OptimizationTierOptimized;
tieredCompilation_DefaultTier = (DWORD)NativeCodeVersion::OptimizationTierOptimized;

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants