[NinoFloris]

Based on #136738

mostly wondering about diffs.

[azure-pipelines]

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.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

[NinoFloris]

@EgorBo diffs with the ifdefs removed stay identical to the positive diffs in #136738 (https://dev.azure.com/dnceng-public/public/_build/results?buildId=1656969&view=ms.vss-build-web.run-extensions-tab)

We can make the switch.

[EgorBo]

@EgorBo diffs with the ifdefs removed stay identical to the positive diffs in #136738 (https://dev.azure.com/dnceng-public/public/_build/results?buildId=1656969&view=ms.vss-build-web.run-extensions-tab)

We can make the switch.

SPMI doesn't show diffs for changes in C#

[EgorBo]

@MihuBot

[NinoFloris]

@EgorBo diffs with the ifdefs removed stay identical to the positive diffs in #136738 (https://dev.azure.com/dnceng-public/public/_build/results?buildId=1656969&view=ms.vss-build-web.run-extensions-tab)
We can make the switch.

SPMI doesn't show diffs for changes in C#

Ah right... How often do the collections get refreshed again? (just for my understanding when it would show up in the perf tracking even, if this was merged hypothetically)

[EgorBo]

@EgorBo diffs with the ifdefs removed stay identical to the positive diffs in #136738 (https://dev.azure.com/dnceng-public/public/_build/results?buildId=1656969&view=ms.vss-build-web.run-extensions-tab)
We can make the switch.

SPMI doesn't show diffs for changes in C#

Ah right... How often do the collections get refreshed again? (just for my understanding when it would show up in the perf tracking even, if this was merged hypothetically)

I don't think that C# change can be merged, I expect quite a lot of regressions from that still

[NinoFloris]

It did look good locally. I did do some jitdisasms and a few fresh PMI runs against ~100k methods (corelib and some libraries).

[EgorBo]

Looking at the diffs - nothing changed since the last time I tried, still quite a lot of regressions (52 improved, 42 regressed). This needs an optimization that recoveres the hack as the last step, it's not straighforward - I wouldn't rely on AI to implement it.

[NinoFloris]

I should have used more local PMI coverage to diff against before claiming it was OK, but it looks worse than it is. My late night mistake.

I have pushed a few more changes, explained below. With these changes, my local ARM64 PMI run shows 4,078 smaller methods versus 2,833 larger, with roughly 31 KB less code overall. About 92% of regressed methods grow by at most 16 bytes, mostly in cases where both bounds comparisons survive in the generated code.

@EgorBo what cost/benefit threshold would you use for this change? Do you consider JIT support for transforming checks back into a widened addition a prerequisite for merging? Transforming the checks into the widened-add form in a later phase wouldn’t undo the effects of suboptimal handling in earlier phases, so we'd need everything then.

First off, a missed check in StringBuilder.Append, which needs to match the new Slice check. A substantial source of the chunkier code size regressions. Once synced, the sizes for regressions in e.g. System.Diagnostics.Debug+AssertInterpolatedStringHandler:AppendFormatted improve to the same size or smaller than before.

runtime/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs

Lines 691 to 695 in 591622c

	// Use the same check as Span<T>.Slice for 64-bit so it can be folded
	// Since repeatCount can't be negative, there's no risk for it to overflow on 32 bit
	if (((nuint)(uint)chunkLength + (nuint)(uint)repeatCount) <= (nuint)(uint)chunkChars.Length)
	{
	chunkChars.AsSpan(chunkLength, repeatCount).Fill(value);

After investigating the regressions, most fall into these categories:

• Heuristic cutoffs: string.Substring grows from seven to eight basic blocks due to the split comparison, going over the BB cutoff. A few string.AsMemory(int, int) inline rejections for the same reasons. Also one or two finally cloning cutoffs in methods, again due to the split check.

• Constant arguments: the widened check folds directly to one comparison, while the subtraction-based form retains two. For nonnegative constants start and count whose sum fits in an int, (uint)length >= start && (uint)(length - start) >= count can be folded to (uint)length >= start + count, but main currently lacks this boolean optimization.

• Repeated length loads: the second load of the string, array, or span length exposed quite some limitations in CSE:

  • Partly fixed by Canonicalize unused field addresses and length reads #136738.
  • Another issue is that CSE can update a copy’s source while the copy retains its old VN, preventing later optimizations from connecting them. We now follow SSA copy definitions when proving values non-null, preserving existing VNs and assertions. This removes a few more redundant null checks on copied receivers.
  • CSE rejected direct reads of address-exposed locals as candidates. Allowing them through lets repeated Span length loads share a captured value, enabling more bounds check elimination.
    I first tried to work around these issues by reading the length into a local in C#, but that then caused regressions on inlining budgets and such everywhere, so it was reverted.

Two more CSE related issues surfaced only after those initial CSE fixes:

• Conditional compares: Once CSE could reuse the repeated length loads, it could embed a temporary assignment inside the first comparison, blocking conditional compare formation (ccmp on ARM64). Separating that assignment from the comparison preserves evaluation order and allows the checks to be combined.

• Exact-type assertions: Once CSE could reuse reads of address-exposed locals, it could give the captured object a new VN while the cached method-table VN still referred to the old identity. This stopped exact-type assertions from matching, leaving redundant type checks and unboxing fallbacks.

  • We now follow the CSE temporary’s defining load when matching those assertions to remove the redundant checks.
  • Alternatively, CSE could keep the affected method-table VNs and their dependent uses consistent with the new receiver identity.
    I picked the first but I’m not sure which approach is preferred here.

I have included all these opts into this PR for diff verification only.

[NinoFloris]

@MihuBot