Skip to content

JIT: less loop inversion for bottom tested loops with no evident IV#130368

Open
AndyAyersMS wants to merge 6 commits into
dotnet:mainfrom
AndyAyersMS:jit-loop-inversion-bottom-tested-benefit
Open

JIT: less loop inversion for bottom tested loops with no evident IV#130368
AndyAyersMS wants to merge 6 commits into
dotnet:mainfrom
AndyAyersMS:jit-loop-inversion-bottom-tested-benefit

Conversation

@AndyAyersMS

@AndyAyersMS AndyAyersMS commented Jul 8, 2026

Copy link
Copy Markdown
Member

Make loop inversion a bit less aggressive.

If a loop is bottom-tested and has no evident IV, only invert if there is a hint that inversion might lead to a beneficial CSE (call or possibly invariant load). Analysis is a approximate and piggy backs on existing IR walks we already do.

Fixes #130045.

Note

This change and PR description were produced with GitHub Copilot CLI.

Under JitLoopInversionRequireBenefitForBottomTested (off by default), skip
inverting an already bottom-tested loop with no recognized IV unless the
duplicated condition holds a call or a loop-invariant, hoistable load.

Targets the arm64 regressions in dotnet#130045 while keeping the Dictionary
Enumerator.MoveNext inversion.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 8, 2026 17:53
@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 8, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@AndyAyersMS

Copy link
Copy Markdown
Member Author

@EgorBot -arm64 --envvars DOTNET_JitLoopInversionRequireBenefitForBottomTested:1

using System;
using System.Numerics;
using BenchmarkDotNet.Attributes;

public class Bench
{
    private readonly int[] _a = new int[512];
    private readonly int[] _b = new int[512];
    private BigInteger _x, _y;

    [GlobalSetup]
    public void Setup()
    {
        for (int i = 0; i < _a.Length; i++) { _a[i] = i; _b[i] = i; }
        byte[] bytes = new byte[259];
        new Random(42).NextBytes(bytes);
        bytes[^1] &= 0x7f;
        _x = new BigInteger(bytes);
        _y = new BigInteger(bytes);
    }

    [Benchmark]
    public bool Span_SequenceEqual() => _a.AsSpan().SequenceEqual(_b);

    [Benchmark]
    public int Span_IndexOf_NotFound() => _a.AsSpan().IndexOf(-1);

    [Benchmark]
    public int BigInteger_CompareTo() => _x.CompareTo(_y);

    [Benchmark]
    public bool BigInteger_Equals() => _x.Equals(_y);
}

Base is main (the env var names a config it doesn't have, so it runs the current #129868 codegen); the PR side has the gate enabled — so the diff isolates the fix on these #130045 cases.

Note

Comment generated with GitHub Copilot CLI.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 adds an (opt-in) heuristic gate to optTryInvertWhileLoop so that when a loop is already bottom-tested and AnalyzeIteration recognizes no IV, loop inversion is only performed if duplicating the condition block appears to have a “benefit” (call or potentially hoistable memory load). It also introduces a new JIT config knob to enable this gate.

Changes:

  • Track “exiting cond latch seen” separately from “IV-test latch” and only apply the new gate in the bottom-tested + no-IV case.
  • Classify condBlock for calls/indirections and use the existing loop size walk to conservatively detect stores to locals used in indirection address expressions.
  • Add JitLoopInversionRequireBenefitForBottomTested config (default off).

Reviewed changes

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

File Description
src/coreclr/jit/optimizer.cpp Adds the benefit-based gate and condition classification for bottom-tested no-IV loops.
src/coreclr/jit/jitconfigvalues.h Introduces the new release config switch controlling the gate.

Comment thread src/coreclr/jit/optimizer.cpp Outdated
Comment thread src/coreclr/jit/optimizer.cpp Outdated
Comment thread src/coreclr/jit/jitconfigvalues.h Outdated
@AndyAyersMS

Copy link
Copy Markdown
Member Author

Re-running on server arm64 (Cobalt 100) — the earlier -arm64 landed on an Apple M4, which doesn't share the #130045 microarchitecture.

@EgorBot -linux_arm64 --envvars DOTNET_JitLoopInversionRequireBenefitForBottomTested:1

using System;
using System.Numerics;
using BenchmarkDotNet.Attributes;

public class Bench
{
    private readonly int[] _a = new int[512];
    private readonly int[] _b = new int[512];
    private BigInteger _x, _y;

    [GlobalSetup]
    public void Setup()
    {
        for (int i = 0; i < _a.Length; i++) { _a[i] = i; _b[i] = i; }
        byte[] bytes = new byte[259];
        new Random(42).NextBytes(bytes);
        bytes[^1] &= 0x7f;
        _x = new BigInteger(bytes);
        _y = new BigInteger(bytes);
    }

    [Benchmark]
    public bool Span_SequenceEqual() => _a.AsSpan().SequenceEqual(_b);

    [Benchmark]
    public int Span_IndexOf_NotFound() => _a.AsSpan().IndexOf(-1);

    [Benchmark]
    public int BigInteger_CompareTo() => _x.CompareTo(_y);

    [Benchmark]
    public bool BigInteger_Equals() => _x.Equals(_y);
}

Base is main (env var names a config it lacks, so it runs the current #129868 codegen); PR side has the gate enabled.

Note

Comment generated with GitHub Copilot CLI.

@AndyAyersMS

Copy link
Copy Markdown
Member Author

Adding the two biggest #130045 regressions (LastIndexOfAnyExcept("ßäöüÄÖÜ"), ContainsKeyTrue<int,int>.IDictionary) plus two anchors, across server arm64 (Cobalt) and x64. Note: the fleet has no Windows Zen4, so Windows here is Turin (Zen5); Linux x64 is Genoa (Zen4).

@EgorBot -linux_arm64 -ubuntu24_azure_genoa -windows_x64 --envvars DOTNET_JitLoopInversionRequireBenefitForBottomTested:1

using System;
using System.Buffers;
using System.Collections.Generic;
using System.Linq;
using System.Numerics;
using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;

public class Bench
{
    private SearchValues<char> _searchValues;
    private char[] _textExcept;

    private int[] _found;
    private Dictionary<int, int> _dictionary;

    private readonly int[] _a = new int[512];
    private readonly int[] _b = new int[512];
    private BigInteger _x, _y;

    [GlobalSetup]
    public void Setup()
    {
        _searchValues = SearchValues.Create("ßäöüÄÖÜ");
        _textExcept = new string('ß', 256).ToCharArray();
        _textExcept[128] = '\n';

        _found = Enumerable.Range(0, 512).ToArray();
        _dictionary = _found.ToDictionary(k => k, k => k);

        for (int i = 0; i < _a.Length; i++) { _a[i] = i; _b[i] = i; }
        byte[] bytes = new byte[259];
        new Random(42).NextBytes(bytes);
        bytes[^1] &= 0x7f;
        _x = new BigInteger(bytes);
        _y = new BigInteger(bytes);
    }

    [Benchmark]
    public int LastIndexOfAnyExcept() => _textExcept.AsSpan().LastIndexOfAnyExcept(_searchValues);

    [Benchmark]
    public bool ContainsKeyTrue_IDictionary() => ContainsKey(_dictionary);

    [MethodImpl(MethodImplOptions.NoInlining)]
    private bool ContainsKey(IDictionary<int, int> collection)
    {
        bool result = false;
        var found = _found;
        for (int i = 0; i < found.Length; i++)
            result ^= collection.ContainsKey(found[i]);
        return result;
    }

    [Benchmark]
    public bool Span_SequenceEqual() => _a.AsSpan().SequenceEqual(_b);

    [Benchmark]
    public int BigInteger_CompareTo() => _x.CompareTo(_y);
}

Base is main (env var names a config it lacks, running the current #129868 codegen); PR side has the gate enabled.

Note

Comment generated with GitHub Copilot CLI.

@AndyAyersMS

Copy link
Copy Markdown
Member Author

Also on Ampere (Neoverse-N1) — the core class behind the arm64-ubuntu lab queue, distinct from Cobalt's N2.

@EgorBot -ubuntu24_azure_ampere --envvars DOTNET_JitLoopInversionRequireBenefitForBottomTested:1

using System;
using System.Buffers;
using System.Collections.Generic;
using System.Linq;
using System.Numerics;
using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;

public class Bench
{
    private SearchValues<char> _searchValues;
    private char[] _textExcept;

    private int[] _found;
    private Dictionary<int, int> _dictionary;

    private readonly int[] _a = new int[512];
    private readonly int[] _b = new int[512];
    private BigInteger _x, _y;

    [GlobalSetup]
    public void Setup()
    {
        _searchValues = SearchValues.Create("ßäöüÄÖÜ");
        _textExcept = new string('ß', 256).ToCharArray();
        _textExcept[128] = '\n';

        _found = Enumerable.Range(0, 512).ToArray();
        _dictionary = _found.ToDictionary(k => k, k => k);

        for (int i = 0; i < _a.Length; i++) { _a[i] = i; _b[i] = i; }
        byte[] bytes = new byte[259];
        new Random(42).NextBytes(bytes);
        bytes[^1] &= 0x7f;
        _x = new BigInteger(bytes);
        _y = new BigInteger(bytes);
    }

    [Benchmark]
    public int LastIndexOfAnyExcept() => _textExcept.AsSpan().LastIndexOfAnyExcept(_searchValues);

    [Benchmark]
    public bool ContainsKeyTrue_IDictionary() => ContainsKey(_dictionary);

    [MethodImpl(MethodImplOptions.NoInlining)]
    private bool ContainsKey(IDictionary<int, int> collection)
    {
        bool result = false;
        var found = _found;
        for (int i = 0; i < found.Length; i++)
            result ^= collection.ContainsKey(found[i]);
        return result;
    }

    [Benchmark]
    public bool Span_SequenceEqual() => _a.AsSpan().SequenceEqual(_b);

    [Benchmark]
    public int BigInteger_CompareTo() => _x.CompareTo(_y);
}

Base is main (env var names a config it lacks, running the current #129868 codegen); PR side has the gate enabled.

Note

Comment generated with GitHub Copilot CLI.

@AndyAyersMS

Copy link
Copy Markdown
Member Author

@EgorBot -ubuntu24_azure_ampere --envvars DOTNET_JitLoopInversionRequireBenefitForBottomTested:1 --filter "ContainsKeyTrueInt32IDictionary"

Real perf-repo benchmark this time (random ValuesGenerator keys). Base is main (env var names a config it lacks → current #129868 codegen); PR side has the gate enabled.

Note

Comment generated with GitHub Copilot CLI.

@AndyAyersMS

Copy link
Copy Markdown
Member Author

Retry of the ContainsKeyTrue<Int32,Int32>.IDictionary filter run (prior attempt returned NA on the PR side with a bare BDN RuntimeError — no exception, and it reproduces neither in a local osx-arm64 run of the same benchmark under the config nor in SPMI replay, so it looks like a transient harness hiccup).

@EgorBot -ubuntu24_azure_ampere --envvars DOTNET_JitLoopInversionRequireBenefitForBottomTested:1 --filter "ContainsKeyTrueInt32IDictionary"

Base is main (env var names a config it lacks → current #129868 codegen); PR side has the gate enabled.

Note

Comment generated with GitHub Copilot CLI.

@AndyAyersMS

Copy link
Copy Markdown
Member Author

EgorBot validation summary

Ran the #130045 cases across the hardware classes behind the perf lab, comparing main (current #129868 codegen) vs this PR with the gate enabled (--envvars DOTNET_JitLoopInversionRequireBenefitForBottomTested:1; main ignores the unknown config, so it serves as the regressed baseline).

Hardware → lab mapping: Ampere N1 = arm64-ubuntu, Cobalt N2 = arm64-azlinux, Genoa Zen4 = Linux/x64, Apple M4 = not a lab target.

The two biggest regressions recover to their lab baselines

Benchmark (hardware) lab baseline → regressed EgorBot: main → PR
ContainsKeyTrue<int,int>.IDictionary (Ampere N1) 3.36 µs → 4.85 µs 5.09 µs → 3.42 µs (−33%)
SearchValuesCharTests.LastIndexOfAnyExcept (Genoa Zen4) 87.8 ns → 131.9 ns 140.1 ns → 99.8 ns (−29%)

The PR numbers land right on the pre-#129868 lab baselines.

Full matrix (fix effect on the benchmark's Mean; negative = faster)

Benchmark Apple M4 Ampere N1 Cobalt N2 Genoa Zen4
BigInteger.CompareTo −44% −49% −36% −21%
Span<int>.SequenceEqual ~0% −15% −6.6% +4.4%
LastIndexOfAnyExcept ~0% ~0% −29%
ContainsKeyTrue.IDictionary −33% ~0% ~0%
Span<int>.IndexOf (not found) +6% +2%

Reading it

  • Every benchmark recovers on the hardware where it actually regressed: ContainsKeyTrue and SequenceEqual on Ampere N1, LastIndexOfAnyExcept on Zen4, BigInteger.CompareTo everywhere.
  • The give-back is small and localized to Span scans on cores where that inversion is genuinely beneficial (SequenceEqual +4% on Zen4, IndexOf +2–6% on M4/N2).
  • ContainsKeyTrue/LastIndexOfAnyExcept show ~0% on non-matching cores because they never regressed there (LastIndexOfAnyExcept was an x64-only regression; it improved on x86).

Notes: ContainsKeyTrue and LastIndexOfAnyExcept use --filter against the real perf-repo benchmarks; the rest are inline repros. One --filter run returned a transient NA on the PR side (no exception; not reproducible locally or in SPMI) and passed on retry.

Note

Comment generated with GitHub Copilot CLI.

Defer the benefit-gate bitvec allocation to when the config is on, correct
the "indirection address locals" wording, and soften the config comment.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 9, 2026 01:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread src/coreclr/jit/optimizer.cpp
Comment thread src/coreclr/jit/jitconfigvalues.h Outdated
AndyAyersMS and others added 2 commits July 14, 2026 11:11
Remove the JitLoopInversionRequireBenefitForBottomTested config and always
apply the gate: for an already bottom-tested loop with no recognized IV, only
invert when the duplicated condition has a call or an indirection whose address
has no loop-varying local.

Also address review feedback: track GT_LCL_ADDR in indirection addresses via
OperIsAnyLocal, and describe the check as an indirection rather than a load.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 14, 2026 22:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 1 out of 1 changed files in this pull request and generated 2 comments.

Comment thread src/coreclr/jit/optimizer.cpp
Comment thread src/coreclr/jit/optimizer.cpp
The message claimed a "hoistable" benefit, but the check only requires a call
or an indirection whose address has no loop-varying local (which does not by
itself prove the load is hoistable). Describe it accurately.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 14, 2026 22:23
@AndyAyersMS AndyAyersMS changed the title JIT: gate inversion of bottom-tested no-IV loops on a hoisting benefit JIT: gate inversion of bottom-tested no-IV loops on a call or non-loop-varying indirection Jul 14, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread src/coreclr/jit/optimizer.cpp Outdated
@AndyAyersMS AndyAyersMS changed the title JIT: gate inversion of bottom-tested no-IV loops on a call or non-loop-varying indirection JIT: less loop inversion for bottom tested loops with no evident IV Jul 15, 2026
@AndyAyersMS
AndyAyersMS marked this pull request as ready for review July 15, 2026 00:54
@AndyAyersMS

Copy link
Copy Markdown
Member Author

@jakobbotsch PTAL
fyi @dotnet/jit-contrib

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@AndyAyersMS
AndyAyersMS requested a review from jakobbotsch July 15, 2026 00:55
@AndyAyersMS

Copy link
Copy Markdown
Member Author

diffs

Comment thread src/coreclr/jit/optimizer.cpp Outdated
Comment on lines +1991 to +1996
// If the loop is already bottom-tested (has an exiting BBJ_COND latch that is not the IV test)
// and no induction variable was recognized, only invert when the block that would be duplicated
// (condBlock) contains a call or an indirection whose address has no loop-varying local. Classify
// condBlock here and record the locals appearing in its indirection address expressions; the
// size-check walk below flags whether any of them is assigned in the loop (making the indirection
// loop-variant).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems very ad-hoc... Is this targeting array lengths specifically or what is the motivating case?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is mainly trying to claw back overly aggressive inversions from #129868. Without a bottom-tested IV, we need some evidence that inversion may help CQ.

The call check is less sensible, let me see if can refine that to just hoistable calls.

Screen a bottom-tested/no-IV loop condition's calls with optIsCSEcandidate
(no persistent side effects, not an allocator) instead of accepting any call,
and require the call's arguments to be loop-invariant, matching the treatment
of indirections. optIsCSEcandidate/CanConsiderTree gain a skipCostChecks arg so
the structural filter can run during loop inversion before tree costs are set.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 16, 2026 21:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 4 out of 4 changed files in this pull request and generated 3 comments.

Comment on lines +2057 to 2061
if (n->OperIsIndir())
{
*m_hasCandidate = true;
CollectLocals(n->AsIndir()->Addr());
}
Comment on lines +2186 to +2189
// Skip the inversion unless the duplicated test carries a benefit: a loop-invariant hoisting
// candidate (a CSE-able call or an indirection whose operands have no loop-varying local), which
// LICM can lift once the body dominates the back-edge. condCandidateStored is left conservatively
// false if the size walk above was skipped or aborted early, keeping the inversion.
Comment on lines +2195 to +2197
JITDUMP("No loop-inversion for " FMT_LP "; already bottom-tested with no recognized IV and no "
"loop-invariant hoisting candidate in the duplicated condition\n",
loop->GetIndex());
@AndyAyersMS

Copy link
Copy Markdown
Member Author

Re-validating the key #130045 cases now that the gate is on by default — the DOTNET_JitLoopInversionRequireBenefitForBottomTested config was removed and calls are now vetted via optIsCSEcandidate (14fabd8). No env var this time: base is main (current #129868 codegen = regressed), PR side is the default-on fix.

@EgorBot -ubuntu24_azure_ampere -linux_arm64 -ubuntu24_azure_genoa

using System;
using System.Buffers;
using System.Collections.Generic;
using System.Linq;
using System.Numerics;
using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;

public class Bench
{
    private SearchValues<char> _searchValues;
    private char[] _textExcept;

    private int[] _found;
    private Dictionary<int, int> _dictionary;

    private readonly int[] _a = new int[512];
    private readonly int[] _b = new int[512];
    private BigInteger _x, _y;

    [GlobalSetup]
    public void Setup()
    {
        _searchValues = SearchValues.Create("ßäöüÄÖÜ");
        _textExcept = new string('ß', 256).ToCharArray();
        _textExcept[128] = '\n';

        _found = Enumerable.Range(0, 512).ToArray();
        _dictionary = _found.ToDictionary(k => k, k => k);

        for (int i = 0; i < _a.Length; i++) { _a[i] = i; _b[i] = i; }
        byte[] bytes = new byte[259];
        new Random(42).NextBytes(bytes);
        bytes[^1] &= 0x7f;
        _x = new BigInteger(bytes);
        _y = new BigInteger(bytes);
    }

    [Benchmark]
    public int LastIndexOfAnyExcept() => _textExcept.AsSpan().LastIndexOfAnyExcept(_searchValues);

    [Benchmark]
    public bool ContainsKeyTrue_IDictionary() => ContainsKey(_dictionary);

    [MethodImpl(MethodImplOptions.NoInlining)]
    private bool ContainsKey(IDictionary<int, int> collection)
    {
        bool result = false;
        var found = _found;
        for (int i = 0; i < found.Length; i++)
            result ^= collection.ContainsKey(found[i]);
        return result;
    }

    [Benchmark]
    public bool Span_SequenceEqual() => _a.AsSpan().SequenceEqual(_b);

    [Benchmark]
    public int BigInteger_CompareTo() => _x.CompareTo(_y);
}

Hardware → lab: Ampere N1 = arm64-ubuntu, Cobalt N2 = arm64 (AzureLinux), Genoa Zen4 = linux-x64.

Note

Comment generated with GitHub Copilot CLI.

@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "14fabd8de5f4d59307a8eb606fc9edc40c8bbd13",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "6d47555c87f335445a547d00da9cd7a2034d94ce",
  "last_reviewed_commit": "14fabd8de5f4d59307a8eb606fc9edc40c8bbd13",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "6d47555c87f335445a547d00da9cd7a2034d94ce",
  "last_recorded_worker_run_id": "29685646334",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "14fabd8de5f4d59307a8eb606fc9edc40c8bbd13",
      "review_id": 4730705724
    }
  ]
}

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Holistic Review

Motivation: Fixes #130045, where loop inversion of already bottom-tested loops with no recognized induction variable caused arm64 regressions. Inverting such loops duplicates the exit condition without a clear payoff, so the change gates that specific case on evidence that duplication will actually enable a downstream optimization.

Approach: In optTryInvertWhileLoop, the existing back-edge scan is refactored to track sawExitingCondLatch separately from the early-out IV-test check. When a loop is bottom-tested with no recognized IV (bottomTestedNoIV), a GenTreeVisitor classifies condBlock for a hoisting benefit — a CSE-able call (via optIsCSEcandidate) or an indirection — and records the operand locals. The existing loop-size complexity walk is reused to detect whether any of those locals is stored in the loop (making the candidate loop-variant). Inversion proceeds only when a benefit exists and none of its operands are stored. To support calling the CSE legality filter before tree costs are initialized, CanConsiderTree/optIsCSEcandidate gain a skipCostChecks parameter that bypasses only the cost-based MIN_CSE_COST gate while preserving all legality/structural checks.

Summary: The change is well-scoped, thoroughly commented, and correct. The gate applies only to the narrow bottomTestedNoIV case, and every conservative fallback (size walk skipped when JitLoopInversionSizeLimit < 0, or aborted early on complexity) is biased toward preserving the prior inverting behavior rather than newly suppressing it. The assert(analyzedIteration) holds because sawExitingCondLatch is only set inside branches that first call isIvTest, which forces AnalyzeIteration. The skipCostChecks refactor is a clean, side-effect-free restructuring of the cost block. Piggy-backing local-store detection on the existing complexity walk keeps the added cost negligible. I have only minor, non-blocking observations noted below. LGTM.

Detailed Findings

Non-blocking observations (not actionable, no changes required):

  1. optcse.cpp — The indirection branch in CondClassifier::PreOrderVisit sets *m_hasCandidate = true for any OperIsIndir node without excluding volatile indirections, which LICM cannot hoist. Because the address-local store check still runs, a genuinely loop-variant indirection is filtered out, but a volatile invariant load would be treated as a benefit. This only makes the gate slightly more permissive (retaining an inversion that may not pay off), so it does not regress correctness or the targeted #130045 scenario. Optional tightening if the heuristic proves too permissive in practice.

  2. optcse.cppoptGetCSEheuristic() now constructs and caches the CSE heuristic during loop inversion (earlier than the CSE phase) for methods that reach the classifier with a call in condBlock. This is harmless — the object is cached in optCSEheuristic and reused unchanged by optOptimizeValnumCSEs — but it is a phase-ordering behavior change worth noting; the heuristic is now instantiated for some methods that previously deferred it until CSE. No functional impact.

Given the heuristic nature of the change, the arm64-focused CI/perf validation referenced in the PR is the right gate for confirming the regression fix and absence of broad CQ loss; the code itself is sound.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 118.6 AIC · ⌖ 14.9 AIC · ⊞ 10K

@AndyAyersMS

Copy link
Copy Markdown
Member Author

@EgorBot -ubuntu24_azure_ampere -linux_arm64 -ubuntu24_azure_genoa

using System;
using System.Buffers;
using System.Collections.Generic;
using System.Linq;
using System.Numerics;
using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;

public class Bench
{
    private SearchValues<char> _searchValues;
    private char[] _textExcept;

    private int[] _found;
    private Dictionary<int, int> _dictionary;

    private readonly int[] _a = new int[512];
    private readonly int[] _b = new int[512];
    private BigInteger _x, _y;

    [GlobalSetup]
    public void Setup()
    {
        _searchValues = SearchValues.Create("ßäöüÄÖÜ");
        _textExcept = new string('ß', 256).ToCharArray();
        _textExcept[128] = '\n';

        _found = Enumerable.Range(0, 512).ToArray();
        _dictionary = _found.ToDictionary(k => k, k => k);

        for (int i = 0; i < _a.Length; i++) { _a[i] = i; _b[i] = i; }
        byte[] bytes = new byte[259];
        new Random(42).NextBytes(bytes);
        bytes[^1] &= 0x7f;
        _x = new BigInteger(bytes);
        _y = new BigInteger(bytes);
    }

    [Benchmark]
    public int LastIndexOfAnyExcept() => _textExcept.AsSpan().LastIndexOfAnyExcept(_searchValues);

    [Benchmark]
    public bool ContainsKeyTrue_IDictionary() => ContainsKey(_dictionary);

    [MethodImpl(MethodImplOptions.NoInlining)]
    private bool ContainsKey(IDictionary<int, int> collection)
    {
        bool result = false;
        var found = _found;
        for (int i = 0; i < found.Length; i++)
            result ^= collection.ContainsKey(found[i]);
        return result;
    }

    [Benchmark]
    public bool Span_SequenceEqual() => _a.AsSpan().SequenceEqual(_b);

    [Benchmark]
    public int BigInteger_CompareTo() => _x.CompareTo(_y);
}

@AndyAyersMS

Copy link
Copy Markdown
Member Author

Checking whether this PR also addresses #130046 (Windows Zen4). The fleet has no Windows Zen4, so best-available Zen4 is Linux Genoa; Windows here is Turin (Zen5) as an anchor.

@EgorBot -ubuntu24_azure_genoa -windows_x64 --envvars DOTNET_JitLoopInversionRequireBenefitForBottomTested:1

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using BenchmarkDotNet.Attributes;

public class Bench
{
    private SortedDictionary<int, int> _sortedDict;
    private Dictionary<string, string> _stringDict;

    private CompareInfo _en;
    private string _text;

    [GlobalSetup]
    public void Setup()
    {
        var d = Enumerable.Range(0, 512).ToDictionary(k => k, k => k);
        _sortedDict = new SortedDictionary<int, int>(d);

        var rnd = new Random(12345);
        var keys = new HashSet<string>();
        while (keys.Count < 512)
            keys.Add(rnd.Next().ToString("X8") + rnd.Next().ToString("X8"));
        _stringDict = keys.ToDictionary(k => k, k => k);

        _en = CultureInfo.GetCultureInfo("en-US").CompareInfo;
        _text = "NET Conf provides a wide selection of live sessions streaming here that feature speakers from the community and .NET product teams. It is a chance to learn, ask questions live, and get inspired for your next software project";
    }

    // System.Collections.CtorFromCollection<Int32>.SortedDictionaryDeepCopy(Size: 512)
    [Benchmark]
    public SortedDictionary<int, int> SortedDictionaryDeepCopy_Int()
        => new SortedDictionary<int, int>(_sortedDict);

    // System.Collections.CtorFromCollection<String>.Dictionary(Size: 512)
    [Benchmark]
    public Dictionary<string, string> Dictionary_String()
        => new Dictionary<string, string>(_stringDict);

    // System.Globalization.Tests.StringSearch.LastIndexOf_Word_NotFound(en-US, OrdinalIgnoreCase, false)
    [Benchmark]
    public int StringSearch_LastIndexOf_Word_NotFound()
        => _en.LastIndexOf(_text, "word", CompareOptions.OrdinalIgnoreCase);
}

Base is main (env var names a config it lacks, so it runs the current #129868 codegen); PR side has the gate enabled by default (commit 0a47d9dd3), and the env var is redundant on this side.

Note

Comment generated with GitHub Copilot CLI.

@AndyAyersMS

Copy link
Copy Markdown
Member Author

Re-running (previous attempt hit an EgorBot arg-parsing glitch on --envvars). Gate is on by default on this branch, so no env var is needed.

@EgorBot -ubuntu24_azure_genoa

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using BenchmarkDotNet.Attributes;

public class Bench
{
    private SortedDictionary<int, int> _sortedDict;
    private Dictionary<string, string> _stringDict;

    private CompareInfo _en;
    private string _text;

    [GlobalSetup]
    public void Setup()
    {
        var d = Enumerable.Range(0, 512).ToDictionary(k => k, k => k);
        _sortedDict = new SortedDictionary<int, int>(d);

        var rnd = new Random(12345);
        var keys = new HashSet<string>();
        while (keys.Count < 512)
            keys.Add(rnd.Next().ToString("X8") + rnd.Next().ToString("X8"));
        _stringDict = keys.ToDictionary(k => k, k => k);

        _en = CultureInfo.GetCultureInfo("en-US").CompareInfo;
        _text = "NET Conf provides a wide selection of live sessions streaming here that feature speakers from the community and .NET product teams. It is a chance to learn, ask questions live, and get inspired for your next software project";
    }

    [Benchmark]
    public SortedDictionary<int, int> SortedDictionaryDeepCopy_Int()
        => new SortedDictionary<int, int>(_sortedDict);

    [Benchmark]
    public Dictionary<string, string> Dictionary_String()
        => new Dictionary<string, string>(_stringDict);

    [Benchmark]
    public int StringSearch_LastIndexOf_Word_NotFound()
        => _en.LastIndexOf(_text, "word", CompareOptions.OrdinalIgnoreCase);
}

Genoa Zen4 is the closest available mapping for #130046's Windows Zen4.

Note

Comment generated with GitHub Copilot CLI.

@AndyAyersMS

Copy link
Copy Markdown
Member Author

@EgorBot -ubuntu24_azure_genoa

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using BenchmarkDotNet.Attributes;

public class Bench
{
    private SortedDictionary<int, int> _sortedDict;
    private Dictionary<string, string> _stringDict;

    private CompareInfo _en;
    private string _text;

    [GlobalSetup]
    public void Setup()
    {
        var d = Enumerable.Range(0, 512).ToDictionary(k => k, k => k);
        _sortedDict = new SortedDictionary<int, int>(d);

        var rnd = new Random(12345);
        var keys = new HashSet<string>();
        while (keys.Count < 512)
            keys.Add(rnd.Next().ToString("X8") + rnd.Next().ToString("X8"));
        _stringDict = keys.ToDictionary(k => k, k => k);

        _en = CultureInfo.GetCultureInfo("en-US").CompareInfo;
        _text = "NET Conf provides a wide selection of live sessions streaming here that feature speakers from the community and .NET product teams. It is a chance to learn, ask questions live, and get inspired for your next software project";
    }

    [Benchmark]
    public SortedDictionary<int, int> SortedDictionaryDeepCopy_Int()
        => new SortedDictionary<int, int>(_sortedDict);

    [Benchmark]
    public Dictionary<string, string> Dictionary_String()
        => new Dictionary<string, string>(_stringDict);

    [Benchmark]
    public int StringSearch_LastIndexOf_Word_NotFound()
        => _en.LastIndexOf(_text, "word", CompareOptions.OrdinalIgnoreCase);
}

Third attempt — checking whether this PR also addresses the #130046 (Windows Zen4) regressions. Genoa Zen4 is the closest available mapping in the fleet. Gate is on by default on this branch, so no env var needed. (Previous two runs hit an EgorBot parser bug that injects a stray t BDN arg when there's any prose before the @EgorBot line — hence putting the trigger line first this time.)

Note

Comment generated with GitHub Copilot CLI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Perf] Linux/x64: 4 Regressions on 6/26/2026 6:28:17 PM +00:00

3 participants