JIT: Fix value profiling in optimized instrumented tiers - #134160
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 853b5c9d-0ee6-4e31-9290-82d2a20aa665
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
🟡 Changes recommended
Add targeted regression coverage and preserve unrolling for constant-length calls.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates RyuJIT value profiling for Memmove and SequenceEqual in optimized instrumented tiers, including inline candidates.
Changes:
- Adds value-profile metadata for both intrinsic paths.
- Handles inline-candidate instrumentation.
- Supports PGO-driven unrolling with default tiering and ReadyToRun.
File summaries
| File | Description |
|---|---|
src/coreclr/jit/importercalls.cpp |
Adds value-profile handling for instrumented intrinsic calls. |
Review details
Suppressed comments (1)
src/coreclr/jit/importercalls.cpp:1560
- This now profiles every Memmove/SequenceEqual call in an optimized instrumented compilation, including calls whose length is already a constant. The value-probe inserter rewrites argument 2 to a comma containing CORINFO_HELP_VALUEPROFILE (fgprofile.cpp:2304-2327), while LowerCallMemmove/LowerCallMemcmp only unroll when that argument is still an integral constant (lower.cpp:2462-2468, 2552-2559). As a result, constant-size copies and comparisons lose their existing unrolling in the instrumented tier; avoid instrumenting constant lengths, and make the schema/visitor safe when a flagged block contains both constant and nonconstant calls.
if (opts.IsInstrumented() && JitConfig.JitProfileValues() && call->IsCall() && call->AsCall()->IsSpecialIntrinsic())
{
const NamedIntrinsic ni = lookupNamedIntrinsic(call->AsCall()->gtCallMethHnd);
if ((ni == NI_System_SpanHelpers_Memmove) || (ni == NI_System_SpanHelpers_SequenceEqual))
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 853b5c9d-0ee6-4e31-9290-82d2a20aa665
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
🔵 Needs a closer look
Three unresolved moderate issues require fixes before approval.
Review details
Suppressed comments (3)
src/coreclr/jit/importercalls.cpp:5546
- Can this new
getHelperFtnquery be made compatible with existing SuperPMI contexts?MethodContext::repGetHelperFtnrequires a recorded entry and fails when the key is missing (src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp:2355-2367), while contexts captured before this change will not contain aCORINFO_HELP_MEMCPYquery for this path. Please use the established replay-compatibility mechanism or construct the existing helper without introducing an unconditional new EE query.
CORINFO_METHOD_HANDLE memmoveHnd = NO_METHOD_HANDLE;
info.compCompHnd->getHelperFtn(CORINFO_HELP_MEMCPY, nullptr, &memmoveHnd);
if (memmoveHnd == NO_METHOD_HANDLE)
src/coreclr/jit/importercalls.cpp:1560
- The existing Memmove/SequenceEqual tests cover semantics and constant-length unrolling, but the new behavior is the value-histogram schema/probe path in optimized instrumented tiers, including inline candidates. The PGO smoke test does not exercise either operation, so a regression in the new
BBF_HAS_VALUE_PROFILEor inline-candidate handoff would pass; add a focused PGO test that warms variable lengths and verifies the profiled unroll or equivalent instrumentation result.
// Collect value profiles in optimized instrumented tiers too, before wrapping inline candidates.
if (opts.IsInstrumented() && JitConfig.JitProfileValues() && call->IsCall() && call->AsCall()->IsSpecialIntrinsic())
{
const NamedIntrinsic ni = lookupNamedIntrinsic(call->AsCall()->gtCallMethHnd);
if ((ni == NI_System_SpanHelpers_Memmove) || (ni == NI_System_SpanHelpers_SequenceEqual))
src/coreclr/jit/importercalls.cpp:3696
- Recognizing NI_System_Buffer_Memmove adds a new call shape, but fgbasic.cpp still raises CALLSITE_UNROLLABLE_MEMOP only for the SpanHelpers Memmove/SequenceEqual intrinsics (src/coreclr/jit/fgbasic.cpp:1140-1153). A constant Buffer.Memmove element count is lowered to the same byte Memmove call here, so inline candidates can miss the unrolling profitability hint and lose the constant unroll this intrinsic enables. Add the Buffer intrinsic to that observation or otherwise preserve the hint.
case NI_System_Buffer_Memmove:
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Clarify generic memmove expansion and helper naming. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 853b5c9d-0ee6-4e31-9290-82d2a20aa665
There was a problem hiding this comment.
🟡 Changes recommended
A critical shared-generic instantiation issue and a missing PGO regression test remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/coreclr/jit/importercalls.cpp:5546
- This introduces a deferred TODO without the issue-linked, searchable form required for tracked work. Please either remove the TODO or attach it to a tracking issue using the repository's prefixed convention (for example,
TODO-JIT:).
// TODO: Rename CORINFO_HELP_MEMCPY to CORINFO_HELP_MEMMOVE to reflect its overlap-safe semantics.
src/coreclr/jit/importercalls.cpp:1560
- This new path is only exercised when TieredPGO produces an instrumented optimized tier, including the case where the Memmove/SequenceEqual call is an inline candidate. The existing memmove/sequence-equal tests cover constant or semantic unrolling, while
InstrumentedTiersis only a generic smoke test; none verifies that the value histogram is collected and then consumed for the optimized tier. Please add a PGO regression test that forces promotion and exercises both the variable-length fallback and the profile-driven unrolled path.
if (opts.IsInstrumented() && JitConfig.JitProfileValues() && call->IsCall() && call->AsCall()->IsSpecialIntrinsic())
{
const NamedIntrinsic ni = lookupNamedIntrinsic(call->AsCall()->gtCallMethHnd);
if ((ni == NI_System_SpanHelpers_Memmove) || (ni == NI_System_SpanHelpers_SequenceEqual))
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
|
@EgorBot -macos_arm -linux_arm -linux_amd --envvars DOTNET_JitDisasm:Copy using System;
using BenchmarkDotNet.Attributes;
public class Bench {
private string _src = null!;
private char[] _dst = null!;
[Params(2, 5, 16, 20, 32, 50, 64)]
public int Length { get; set; }
[GlobalSetup]
public void Setup() {
_src = new string('x', Length);
_dst = new char[Length];
}
[Benchmark]
public void Copy() => _src.AsSpan().CopyTo(_dst);
}Note Benchmark snippet generated with GitHub Copilot. |
|
PTAL @AndyAyersMS @dotnet/jit-contrib This PGO-driven memmove finally works (after Andy's PR to instrument inlinees + this PR). Normally, PS: I'll file a PR to rename SPMI and MihaBot aren't showing diffs because this needs real PGO profile. |
Since #134160 led to **69 benchmarks** improved, I decided to do the same for memcmp idiom. ### Benchmark ```cs using BenchmarkDotNet.Attributes; public class Bench { private byte[] _bytes1 = null!; private byte[] _bytes2 = null!; [Params(1, 2, 5, 8, 16, 20, 32, 50, 64, 128)] public int Length { get; set; } [GlobalSetup] public void Setup() { _bytes1 = new byte[Length]; _bytes2 = new byte[Length]; } [Benchmark] public bool Bytes() => _bytes1.AsSpan().SequenceEqual(_bytes2); } ``` Results: EgorBot/Benchmarks#612 (arm64 only handles up to 32 bytes today). --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e9c37cab-1590-4d11-a950-db4e7f065a3d
Collect
Memmove/SequenceEquallength profiles in optimized instrumented tiers, including inline candidates.This enables the existing PGO-driven unrolling with default tiering and ReadyToRun enabled.
Before (Windows x64, Tier1):
After:
Benchmark
Results from EgorBot/Benchmarks#600. Speedup is main / PR; values above 1.10X are bolded, and values below 1.00X indicate regressions.
Lengthis in characters.