JIT: cross-block local assertion prop in morph #94363

AndyAyersMS · 2023-11-03T23:25:10Z

During global morph, allow assertions to propagate to a block from the block's predecessors. Handle special cases where we can't allow this to happen:

block has preds that have not yet been morphed
block has no preds
block is specially flagged as one that might gain new preds during morph
block is an EH handler entry

Contributes to #93246.

For now this is off by default.

During global morph, allow assertions to propagate to a block from the block's predecessors. Handle special cases where we can't allow this to happen: * block has preds that have not yet been morphed * block has no preds * block is specially flagged as one that might gain new preds during morph * block is an EH handler entry Contributes to dotnet#93246.

ghost · 2023-11-03T23:25:23Z

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

Issue Details

During global morph, allow assertions to propagate to a block from the block's predecessors. Handle special cases where we can't allow this to happen:

block has preds that have not yet been morphed
block has no preds
block is specially flagged as one that might gain new preds during morph
block is an EH handler entry

Contributes to #93246.

Author:	AndyAyersMS
Assignees:	AndyAyersMS
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-11-03T23:27:13Z

@jakobbotsch @dotnet/jit-contrib here's the "rough cut"

This will have about a 1% TP impact (win x64) and big code size improvements and regressions. Seems like it will take some time to chip away at those.

Diffs

Running this now to flush out any remaining bugs.

AndyAyersMS · 2023-11-04T01:12:20Z

@amanasifkhalid FYI I'm seeing the same sort of cloning issue you hit in this PR... there are 4600 size regressions so it would be nice to filter out the ones where there was extra cloning.

[18:08:46] Top method regressions (percentages):
[18:08:46]          216 (67.50% of base) : 18807.dasm - Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpHeaders:ValidateHeaderValueCharacters(System.String,Microsoft.Extensions.Primitives.StringValues,System.Func`2[System.String,System.Text.Encoding]) (Tier1)

AndyAyersMS · 2023-11-06T17:10:49Z

Runtime failure is in the diagnostics test that looks at debug info from an optimized method.

…too fast

AndyAyersMS · 2023-11-07T18:46:17Z

Latest commit upscales the bit vectors proportional to lvaCount until we hit the tracked local limit (so using similar sized bit vectors to what we use for liveness, since empirically I see about 0.6 local assertions / local, and want to over-size a bit to reduce likelihood of running out of table space).

This will incur a bit more TP cost; locally looks like around 1.3% or so. But as a benefit we see considerably larger overall code size savings; x64 windows should be around -1MB.

Thoughts about how to get this to the point where it can be merged

Throughput:

I think I can claw some of the TP back by

(1) optimizing search loops in assertion prop. We should generally never search the entire table (save when adding an assertion) and instead walk the live assertion set, and for local prop we should intersect that set with the dep vector.

The bit vector operations aren't free so it might make sense to do this only when there are sufficient numbers of assertions, though that makes the code uglier. Perhaps it can all be hidden behind a suitably smart enumerator.

(2) stop morphing blocks that are unreachable or become unreachable because of local assertion prop.

Aside from removing all the morph related overhead, I suspect sometimes this dead block morphing creates issues for live blocks, eg either useless assertions that cause the table to fill faster, or else global actions (say DNER) that are hard to undo.

Currently the RPO strategy won't allow for removal of dead EH and may or may not make it easy to remove unreachable cyclic flow.

Both of these can be spun off as preliminary checkins, though the full benefits may not be seen without this change, as main has small bit vectors and isn't able to propagate constants nearly as aggressively.

At the end of the day though I expect there will still be a TP impact.

Code Size / Code Quality

With the massive numbers of diffs any sort of manual assessment is going to be a random sampling at best. I will try and look at some of the bigger regressions, possibly suppressing cloning to help remove that as a factor.

The more aggressive copy prop that this enables puts pressure on LSRA, as copies to temps provide natural split points. I am not sure we can find effective heuristics to counterbalance this.

I think it makes sense to check all this in initially as off-by-default, and enable runs in the perf lab, and use that data to both identify CQ regressions and to decide if the CQ improvements justify the additional TP costs.

AndyAyersMS · 2023-11-08T18:50:39Z

Diffs @ bc59375.

Aggregate size savings look good, but still quite a lot of churn, and a fair number of large regressions.

AndyAyersMS · 2023-11-08T23:29:58Z

I'm going to disable this by default for now and set up some lab runs for correctness. I will alsoset up runs of this in a fork in the perf lab, so I can get a sense of the perf impact of all this CQ churn.

In the meantime I'll continue to look at fixing the bad diffs (and bad perf, once I have data) and whittling away at TP, so once this is merged I'll open a draft PR to enable and iterate there.

BruceForstall · 2023-11-09T00:01:59Z

src/tests/Common/testenvironment.proj

@@ -220,6 +221,7 @@
    <TestEnvironment Include="jitobjectstackallocation" JitObjectStackAllocation="1" TieredCompilation="0" />
    <TestEnvironment Include="jitphysicalpromotion_only" JitStressModeNames="STRESS_NO_OLD_PROMOTION" TieredCompilation="0" />
    <TestEnvironment Include="jitphysicalpromotion_full" JitStressModeNames="STRESS_PHYSICAL_PROMOTION_COST STRESS_NO_OLD_PROMOTION" TieredCompilation="0" />
+    <TestEnvironment Include="jitcrossblocklocalassertionprop" JitDoCrossBlockLocalAssertionProp="1" TieredCompilation="0" />


TieredCompilation=0 is the default in this file, so you don't need to specify it (looks like some other cases also over-specify it)

BruceForstall · 2023-11-09T00:03:34Z

src/coreclr/jit/jitconfigvalues.h

@@ -653,6 +653,9 @@ CONFIG_INTEGER(JitEnableHeadTailMerge, W("JitEnableHeadTailMerge"), 1)
 // Enable physical promotion
 CONFIG_INTEGER(JitEnablePhysicalPromotion, W("JitEnablePhysicalPromotion"), 1)

+// Enable cross-block local assertion prop
+CONFIG_INTEGER(JitDoCrossBlockLocalAssertionProp, W("JitDoCrossBlockLocalAssertionProp"), 0)


Do seems fine, but should we use Enable instead, to match other similar cases (like JitEnablePhysicalPromotion, above)?

Sure... it was up with the other Do cases before, but now that I moved it to be available in release it no longer matches its neighbors.

BruceForstall · 2023-11-09T00:08:15Z

src/coreclr/jit/morph.cpp

@@ -13822,7 +13900,8 @@ PhaseStatus Compiler::fgMorphBlocks()

    // Local assertion prop is enabled if we are optimized
    //
-    optLocalAssertionProp = opts.OptimizationEnabled();
+    optLocalAssertionProp           = opts.OptimizationEnabled();
+    optCrossBlockLocalAssertionProp = optLocalAssertionProp;


Suggested change

optCrossBlockLocalAssertionProp = optLocalAssertionProp;

optCrossBlockLocalAssertionProp = optLocalAssertionProp && JitConfig.JitDoCrossBlockLocalAssertionProp();

?

I see you consult the config in optAssertionInit. Is there a reason to wait until then?

I went back and forth as to whether all this setup should be assertion prop's business; I could push more of the logic there I suppose.

BruceForstall · 2023-11-09T00:13:18Z

src/coreclr/jit/morph.cpp

+
+            if (!canUsePredAssertions)
+            {
+                apLocal = BitVecOps::MakeEmpty(apTraits);


Does this also need optAssertionReset(0);?

Can the (!optCrossBlockLocalAssertionProp) and the "failure" cases be combined (tail merged)?

Does this also need optAssertionReset(0);?

No. There are 3 modes of operation here:

classic: block starts with empty table via Reset(0) and empty BV

cross-block: block starts with current table and intersection of pred BVs (which may end up empty)

cross-block special case: block starts with current table and empty BV

Modes 2 and 3 can mix with each other, but not with mode 1.

We use mode 1 if the feature is disabled, or if the method has a lot of locals.

When the feature is enabled, and the method does not have a lot of locals, we use mode 2 (if canUsePredAssertions) or mode 3 (otherwise) depending on the various block properties.

Can the (!optCrossBlockLocalAssertionProp) and the "failure" cases be combined (tail merged)?

Let me look at tweaking this.

BruceForstall · 2023-11-09T00:13:51Z

src/coreclr/jit/morph.cpp

+                    canUsePredAssertions = false;
+                }
+
+                // If there were no preds, there are no asserions in.


Suggested change

// If there were no preds, there are no asserions in.

// If there were no preds, there are no assertions in.

BruceForstall · 2023-11-09T00:15:13Z

src/coreclr/jit/morph.cpp

+                    //
+                    JITDUMP(FMT_BB " pred " FMT_BB " not processed; clearing assertions in\n", block->bbNum,
+                            pred->bbNum);
+                    canUsePredAssertions = false;


add break;?

Or better, make the failure case first, with break, and eliminate the continue

jakobbotsch · 2023-11-09T22:58:22Z

src/coreclr/jit/assertionprop.cpp

+        if (lvaCount > maxTrackedLocals)
+        {
+            JITDUMP("Disabling cross-block assertion prop: too many locals\n");
+            optCrossBlockLocalAssertionProp = false;
+        }


This seems like it might be very "spurious diff" inducing (add a local and suddenly a bunch of assertions stop being propagated). You might have mentioned it somewhere else, but do you have any plans to make this less "hard threshold-y"?

Also looking at things like optAssertionDep below it kind of feels like we should be calling lvaSortByRefCount here and utilize tracked indices, although I don't think ref counts are up to date here (in particular ones introduced by physical promotion -- might not be so hard to fix that).

I guess it's one of those cases where it would be great to have separated "single block" locals and "global" locals.

do you have any plans to make this less "hard threshold-y"?

I don't have any great ideas here, no. Some of this is the inherent cost of a blind forward scheme, we push facts along hoping they will matter. SSA would be helpful, we could just attach assertions to SSA lifetimes.

utilize tracked indices

Some kind of prioritization scheme might work out nicely; presumably frequently referenced locals are more interesting candidates for assertions.

I guess it's one of those cases where it would be great to have separated "single block" locals and "global" locals.

I had the same thought -- I tried an experiment to see how many assertions were killed in the same block they were born in, and it was quite small; most of them get killed at merges.

It might be possible to leverage RCS_EARLY to deduce if newly generated assertions are likely to be referenced later on; if not they could be purged before they "escape" the block. But we're also enabling cross-block copy prop so even getting that right might be tricky, as the ref counts can and will change during morph.

jakobbotsch

LGTM.

AndyAyersMS · 2023-11-10T01:11:33Z

There's an unexpected diff on 32 bit platforms, I want to chase this down... not obvious to me what this is from the code changes. I can repro locally.

AndyAyersMS · 2023-11-10T02:01:56Z

There's an unexpected diff on 32 bit platforms, I want to chase this down... not obvious to me what this is from the code changes. I can repro locally.

Ah, this is fixing an issue I introduced in #94322. In optAssertionIsNonNullInternal we were inadvertently removing an assertion from the dep vector if it was no longer live (via IntersectionD). If it later came back to life we might not be able to find it since it would not be re-added to the dep vector (adding only happens when an assertion is first created). So we might fail to kill it if then again went dead.

We now use Intersection to avoid modifying the dep vector.

My last round of revisions to dotnet#94363 introduced a bug. Fix.

My last round of revisions to #94363 introduced a bug. Fix.

When we expand QMARKS, ensure that any block with a no-return call gets changed to BBJ_THROW. This fixes a case I am seeing with cross-block local assertion prop, as the upper QMARK gets optimized away and so we don't check if the expansing has any noreturn calls. It also happens in places with just within-block local assertion prop. Contributes to dotnet#94363.

When we expand QMARKS, ensure that any block with a no-return call gets changed to BBJ_THROW. This fixes a case I am seeing with cross-block local assertion prop, as the upper QMARK gets optimized away and so we don't check if the expansing has any noreturn calls. It also happens in places with just within-block local assertion prop. Contributes to #94363.

ghost assigned AndyAyersMS Nov 3, 2023

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 3, 2023

AndyAyersMS marked this pull request as draft November 3, 2023 23:25

This was referenced Nov 4, 2023

Timeout in System.Net.Quic.Functional.Tests #86019

Closed

CI error: System.Net.Quic.QuicException: The connection timed out from inactivity #91757

Closed

AndyAyersMS mentioned this pull request Nov 6, 2023

JIT: use reverse post-order (RPO) traversal for morph #93246

Closed

12 tasks

amanasifkhalid mentioned this pull request Nov 6, 2023

JIT: Remove BBJ_NONE #94239

Merged

AndyAyersMS added 2 commits November 6, 2023 19:01

no cross block for methods with lots of locals -- the table fills up …

cceb9f8

…too fast

assertion limit scaling

bc59375

disable by default (for now), enable in jit-experimental

2b78bdc

AndyAyersMS marked this pull request as ready for review November 8, 2023 23:30

BruceForstall reviewed Nov 9, 2023

View reviewed changes

build-analysis bot mentioned this pull request Nov 9, 2023

Curl fails to download dotnet-install.sh dotnet/dnceng#1372

Closed

3 tasks

review feedback

8fa2318

build-analysis bot mentioned this pull request Nov 9, 2023

Crash in Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.WorkItemExecution #90019

Open

BruceForstall approved these changes Nov 9, 2023

View reviewed changes

jakobbotsch reviewed Nov 9, 2023

View reviewed changes

jakobbotsch approved these changes Nov 9, 2023

View reviewed changes

AndyAyersMS merged commit 5127e07 into dotnet:main Nov 10, 2023
195 of 197 checks passed

AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this pull request Nov 10, 2023

JIT: fix cross block assertion prop

573e6db

My last round of revisions to dotnet#94363 introduced a bug. Fix.

AndyAyersMS mentioned this pull request Nov 10, 2023

JIT: fix cross block assertion prop #94608

Merged

AndyAyersMS added a commit that referenced this pull request Nov 10, 2023

JIT: fix cross block assertion prop (#94608)

7c8fff9

My last round of revisions to #94363 introduced a bug. Fix.

AndyAyersMS mentioned this pull request Nov 14, 2023

JIT: consistently handle no return calls in qmarks #94690

Merged

github-actions bot locked and limited conversation to collaborators Dec 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: cross-block local assertion prop in morph #94363

JIT: cross-block local assertion prop in morph #94363

AndyAyersMS commented Nov 3, 2023 •

edited

Loading

ghost commented Nov 3, 2023

AndyAyersMS commented Nov 3, 2023 •

edited

Loading

AndyAyersMS commented Nov 4, 2023

AndyAyersMS commented Nov 6, 2023

AndyAyersMS commented Nov 7, 2023

AndyAyersMS commented Nov 8, 2023

AndyAyersMS commented Nov 8, 2023

BruceForstall Nov 9, 2023

BruceForstall Nov 9, 2023

AndyAyersMS Nov 9, 2023

BruceForstall Nov 9, 2023

BruceForstall Nov 9, 2023

AndyAyersMS Nov 9, 2023

BruceForstall Nov 9, 2023

AndyAyersMS Nov 9, 2023

BruceForstall Nov 9, 2023

BruceForstall Nov 9, 2023

jakobbotsch Nov 9, 2023

jakobbotsch Nov 9, 2023 •

edited

Loading

jakobbotsch Nov 9, 2023

AndyAyersMS Nov 10, 2023

jakobbotsch left a comment

AndyAyersMS commented Nov 10, 2023

AndyAyersMS commented Nov 10, 2023 •

edited

Loading

	optCrossBlockLocalAssertionProp = optLocalAssertionProp;
	optCrossBlockLocalAssertionProp = optLocalAssertionProp && JitConfig.JitDoCrossBlockLocalAssertionProp();

	// If there were no preds, there are no asserions in.
	// If there were no preds, there are no assertions in.

JIT: cross-block local assertion prop in morph #94363

JIT: cross-block local assertion prop in morph #94363

Conversation

AndyAyersMS commented Nov 3, 2023 • edited Loading

ghost commented Nov 3, 2023

AndyAyersMS commented Nov 3, 2023 • edited Loading

AndyAyersMS commented Nov 4, 2023

AndyAyersMS commented Nov 6, 2023

AndyAyersMS commented Nov 7, 2023

Thoughts about how to get this to the point where it can be merged

AndyAyersMS commented Nov 8, 2023

AndyAyersMS commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch left a comment

Choose a reason for hiding this comment

AndyAyersMS commented Nov 10, 2023

AndyAyersMS commented Nov 10, 2023 • edited Loading

AndyAyersMS commented Nov 3, 2023 •

edited

Loading

AndyAyersMS commented Nov 3, 2023 •

edited

Loading

jakobbotsch Nov 9, 2023 •

edited

Loading

AndyAyersMS commented Nov 10, 2023 •

edited

Loading