JIT: less loop inversion for bottom tested loops with no evident IV#130368
JIT: less loop inversion for bottom tested loops with no evident IV#130368AndyAyersMS wants to merge 5 commits into
Conversation
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>
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@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 Note Comment generated with GitHub Copilot CLI. |
There was a problem hiding this comment.
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
condBlockfor calls/indirections and use the existing loop size walk to conservatively detect stores to locals used in indirection address expressions. - Add
JitLoopInversionRequireBenefitForBottomTestedconfig (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. |
|
Re-running on server arm64 (Cobalt 100) — the earlier @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 Note Comment generated with GitHub Copilot CLI. |
|
Adding the two biggest #130045 regressions ( @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 Note Comment generated with GitHub Copilot CLI. |
|
Also on Ampere (Neoverse-N1) — the core class behind the @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 Note Comment generated with GitHub Copilot CLI. |
|
@EgorBot -ubuntu24_azure_ampere --envvars DOTNET_JitLoopInversionRequireBenefitForBottomTested:1 --filter "ContainsKeyTrueInt32IDictionary" Real perf-repo benchmark this time (random Note Comment generated with GitHub Copilot CLI. |
|
Retry of the @EgorBot -ubuntu24_azure_ampere --envvars DOTNET_JitLoopInversionRequireBenefitForBottomTested:1 --filter "ContainsKeyTrueInt32IDictionary" Base is Note Comment generated with GitHub Copilot CLI. |
EgorBot validation summaryRan the #130045 cases across the hardware classes behind the perf lab, comparing Hardware → lab mapping: Ampere N1 = The two biggest regressions recover to their lab baselines
The PR numbers land right on the pre-#129868 lab baselines. Full matrix (fix effect on the benchmark's Mean; negative = faster)
Reading it
Notes: 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>
…bottom-tested-benefit
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>
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>
|
@jakobbotsch PTAL |
|
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. |
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.