JIT: introduce some SSA accounting and checking by AndyAyersMS · Pull Request #77055 · dotnet/runtime

AndyAyersMS · 2022-10-14T18:20:20Z

Record some information about each SSA def and try and keep it conservatively correct through the first few optimization phases.

We note:

total number of uses
whether all uses are in the same block as the def
whether there are any phi uses

Subsequent phases that introduce new uses must now call optRecordSsaUses on the new trees they create to update these accounts.

This information is cross-checked versus the IR in post phase checking. Because we don't have a well defined mechanism to track when nodes are deleted the recorded counts may end up being overestimates in subsequent phases. This is ok, but underestimates are flagged as errors.

ghost · 2022-10-14T18:20:36Z

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

Issue Details

Record some information about each SSA def and try and keep it conservatively correct through the first few optimization phases.

We note:

total number of uses
whether all uses are in the same block as the def
whether there are any phi uses

Subsequent phases that introduce new uses must now call optRecordSsaUses on the new trees they create to update these accounts.

This information is cross-checked versus the IR in post phase checking. Because we don't have a well defined mechanism to track when nodes are deleted the recorded counts may end up being overestimates in subsequent phases. This is ok, but underestimates are flagged as errors.

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

AndyAyersMS · 2022-10-14T18:25:05Z

@jakobbotsch PTAL
cc @dotnet/jit-contrib

I expect to leverage this to re-enable parts of PHI based RBO (we can safely optimize cases where no SSA update is needed). It might also be interesting to rely on it to do another round of forward sub, though it's not clear how valuable that might be.

I currently give up SSA checking after assertion prop because it broadly remorphs and remorphing can duplicate uses. Might not be too hard to extend past this and basically check all phases, though the info may end up being quite conservative.

if we expect to consume this new SSA info later than RBO we could also consider using the checker data to update the SSA info.

This passes SPMI locally with no diffs. Let's see how well it survives jit stress.

SingleAccretion · 2022-10-14T18:30:33Z

I believe all optimization phases (save copy propagation) can call morph an arbitrary statements.

I think the ones upstream from AP are handled now. Assertion prop does this remorphing centrally and globally so it we would end up rescanning more stuff, and there's no expected downstream consumer (other than checking). Easy enough to try adding it later if we think there's value.

I see. There is a sort of "SSA checker" in range check, would be nice to delete that, but I suppose it would require diverging between debug and release code (which seems undesirable for release-available data).

SingleAccretion · 2022-10-14T18:33:57Z

Is it intentional these fields are not under #ifdef DEBUG? They will increase memory consumption quite a bit.

Yes, I plan to rely on this information during RBO.

How much of this information will be required in release? Can it be compressed to avoid the memory regression (by stashing the bits in some block/node pointers, for example)? It does not seem necessary to increase the size by 8 bytes, 4 should suffice for the number of uses + booleans turned bitfields.

RBO will (at least initially) just use m_hasGlobalUse. A possible second pass of forward sub would also check m_numUses though it would only want to know if the value is 0, 1, or > 1.
I wonder how hard it would be for SPMI to track jit memory usage and report that as a release-mode metric. Seems like it should be doable.

Indeed, it would be nice to have this information in CI.
Meanwhile, quick runs over CoreLib yield this (for an equivalent change):
./diff-mem x64 Absolute difference is: 3876656 Relative difference is: 0.185% ./diff-mem x86 Absolute difference is: 3798960 Relative difference is: 0.270%

SingleAccretion · 2022-10-14T18:36:54Z

My experience with optCopyProp tells me this additional parameter could show up on the PIN radar.

AndyAyersMS · 2022-10-14T19:19:55Z

Formatting leg failed to install dotnet:

Tool to install: .NET Core sdk version 3.x.
##[error]Failed to download or parse releases-index.json with error: {"errno":"ETIMEDOUT","code":"ETIMEDOUT","syscall":"connect","address":"52.239.161.42","port":443}
Finishing: Install .NET SDK

OSX build failed during package restore

2022-10-14T19:02:33.0467530Z Determining projects to restore...
2022-10-14T19:02:41.8034380Z Restored /Users/runner/work/1/s/src/tests/Common/external/external.csproj (in 7.66 sec).
2022-10-14T19:02:41.8307520Z /Users/runner/work/1/s/src/tests/build.proj(437,5): error MSB3073: The command ""/Users/runner/work/1/s/.dotnet/dotnet" restore -r osx-x64 Common/external/external.csproj /p:SetTFMForRestore=true /p:TargetOS=OSX /p:TargetArchitecture=x64 /p:Configuration=Checked /p:CrossBuild=" exited with code 1.
2022-10-14T19:02:41.8471380Z 2022-10-14T19:02:41.8472720Z Build FAILED.

SingleAccretion · 2022-10-14T19:37:29Z

(lclNum < m_compiler->lvaCount)
Isn't this always true?

Thanks for spotting this. It was meant to track the cases where SSA creates a virtual def, but I don't yet have the right checks for that.

FWIW, the current exclusions look right to me.
Notably, there exists ambiguity between a virtual definition, and a real definition made by a call in the first block (because calls don't get recorded in m_asg, even as they perhaps should), but that does not seem relevant here.

Yeah, that whole area is a bit messy. Assuming no other feedback I may leave this as is and clean it up later.

AndyAyersMS · 2022-10-14T19:55:59Z

Installer windows_x86 hitting CI issues:

git --config-env=http.extraheader=env_var_http.extraheader fetch --force --tags --prune --prune-tags --progress --no-recurse-submodules origin --depth=20 +61a4209aef80833e15c24ac7b36c95768a1516fe:refs/remotes/origin/61a4209aef80833e15c24ac7b36c95768a1516fe
fatal: unable to access 'https://github.com/dotnet/runtime/': Could not resolve host: github.com
##[warning]Git fetch failed with exit code 128, back off 2.697 seconds before retry.
git --config-env=http.extraheader=env_var_http.extraheader fetch --force --tags --prune --prune-tags --progress --no-recurse-submodules origin --depth=20 +61a4209aef80833e15c24ac7b36c95768a1516fe:refs/remotes/origin/61a4209aef80833e15c24ac7b36c95768a1516fe
fatal: unable to access 'https://github.com/dotnet/runtime/': Could not resolve host: github.com
##[warning]Git fetch failed with exit code 128, back off 8.533 seconds before retry.
git --config-env=http.extraheader=env_var_http.extraheader fetch --force --tags --prune --prune-tags --progress --no-recurse-submodules origin --depth=20 +61a4209aef80833e15c24ac7b36c95768a1516fe:refs/remotes/origin/61a4209aef80833e15c24ac7b36c95768a1516fe
fatal: unable to access 'https://github.com/dotnet/runtime/': Could not resolve host: github.com
##[error]Git fetch failed with exit code: 128

AndyAyersMS · 2022-10-14T21:29:05Z

Per diffs TP impact seems pretty minimal.

AndyAyersMS · 2022-10-14T21:31:24Z

Jitstress is fairly clean, so I'll give it a go and see if it trips up the checker anywhere.

AndyAyersMS · 2022-10-14T21:31:34Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2022-10-14T21:31:49Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2022-10-15T00:49:06Z

I expect to leverage this to re-enable parts of PHI based RBO (we can safely optimize cases where no SSA update is needed).

Local runs of RBO on top of this suggest it gets about 2/3 of the cases, which is much better than I'd expected.

AndyAyersMS · 2022-10-15T02:05:18Z

The only jitstress failure is GitHub_23159, which is #76880.

SingleAccretion · 2022-10-15T15:05:04Z

Nit: the duplication of default values in constructors for these can be avoided with member initializers, i. e.
// The GT_ASG node that generates the definition, or nullptr for definitions // of uninitialized variables. GenTreeOp* m_asg = nullptr;
etc.

Always forget about those newfangled C++ features.

Unfortunately, you can't do this yet for bitfields, at least not without C++ 20 support.

AndyAyersMS · 2022-10-19T18:25:15Z

Let's try and get #76139 merged first. Then I'll come back and revise this.

BruceForstall

Noticed a couple typos while scanning. Looks like it needs updating after recent SSA number change.

Record some information about each SSA def and try and keep it conservatively correct through the first few optimization phases. We note: * total number of uses * whether all uses are in the same block as the def * whether there are any phi uses Subsequent phases that introduce new uses must now call `optRecordSsaUses` on the new trees they create to update these accounts. This information is cross-checked versus the IR in post phase checking. Because we don't have a well defined mechanism to track when nodes are deleted the recorded counts may end up being overestimates in subsequent phases. This is ok, but underestimates are flagged as errors.

AndyAyersMS · 2022-10-29T01:39:27Z

Sorry for the force push. Updated to try and handle the new composite SSA nums.

Downstream consumers will have to be careful as num uses may not be an over-estimate for promoted fields since some implicit uses won't be accounted for. Haven't decided whether to try and fold that into the accounting here or just leave it as caveat emptor.

SingleAccretion · 2022-10-29T16:02:24Z

+ LclVarDsc* const fieldVarDsc = m_compiler->lvaGetDesc(fieldLclNum);
+ unsigned const fieldSsaNum = lclNode->GetSsaNum(m_compiler, index);
+
+ if (isFullDef)


This can be simplified to not check isFullDef, the gtStoreDefinesField path should work for it too.
(The checking of isFullDef in SSA is a TP optimization)

SingleAccretion · 2022-10-29T16:04:11Z

+ }
+ else
+ {
+ isFieldUse = true;


It does not seem right to have this be treated as a use. If the store doesn't define a given field, we'd always expect RESERVED_SSA_NUM for it.

SingleAccretion · 2022-10-29T16:17:00Z

+ // SSA will be associated with the first field if it can replace the var.
+ //
+ if (!varDsc->lvInSsa && varDsc->CanBeReplacedWithItsField(m_compiler))
+ {
+ lclNum = varDsc->lvFieldLclStart;
+ varDsc = m_compiler->lvaGetDesc(lclNum);
+ }


Suggested change
// SSA will be associated with the first field if it can replace the var.
//
if (!varDsc->lvInSsa && varDsc->CanBeReplacedWithItsField(m_compiler))
{
lclNum = varDsc->lvFieldLclStart;
varDsc = m_compiler->lvaGetDesc(lclNum);
}
Since the composite model doesn't record SSA numbers for uses.
(Notably, in the old model, we would never have encountered a CanBeReplacedWithItsField use, only simple full definitions)

SingleAccretion · 2022-10-29T16:18:02Z

+
+ if (isUse)
+ {
+ unsigned lclNum = tree->GetLclNum();


TP: checking for HasSsaName directly would be quicker than going through lvInSsa.

SingleAccretion · 2022-10-29T16:23:50Z

+ const bool isDef = (tree->gtFlags & GTF_VAR_DEF) != 0;
+ const bool isUse = !isDef || ((tree->gtFlags & GTF_VAR_USEASG) != 0);


Handling partial definitions makes the specification comment at the top Ignores SSA defs not quite true. It also implies a need to handle composite definitions.
It is not clear to me why we need to handle them, partial definitions are much like ordinary definitions from SSA point of view (they cannot be introduced or deleted post-SSA).

SingleAccretion · 2022-10-29T16:26:43Z

+
+ // Handle multiple uses
+ //
+ if (tree->HasCompositeSsaName())


Naturally, this will always be false... It would also need some tweaks if we do start renaming SSA uses, in particular, it would need to deduce which fields are actually references by the use (otherwise the "field local is in SSA => has SSA num" assertion won't be true). Delete for now?
(Or is something else planned for this?)

I changed this to assert it is always false, so we'll remember to fix things if we ever decide to try and rename uses.

SingleAccretion · 2022-10-29T16:34:39Z

+ {
+ // If the var is not in ssa, the local should not have an ssa num.
+ //
+ if (!varDsc->lvInSsa)


Nit: it is unusual to use lvInSsa directly, all other code goes through lvaInSsa.

AndyAyersMS · 2022-10-30T23:13:23Z

Failure should be unrelated, am going to retry.

AndyAyersMS · 2022-10-31T15:23:26Z

@dotnet/jit-contrib this is revised after the composite SSA number change.
@BruceForstall since you've already taken a look, how about another?

TP impact is a bit higher than I'd like, from walking some of the bigger trees looking for new SSA uses. We might consider more intrusive ways of looking for new/changed trees. But this information will prove useful in enabling parts of phi-based RBO and possibly a second round of forward sub.

BruceForstall · 2022-10-31T16:15:33Z

Diffs

BruceForstall

LGTM. In general, it could benefit from a few more comments, especially "overview of the algorithm" kind of comments, IMO.

BruceForstall · 2022-10-31T23:26:14Z

+// * Operands that should not have SSA numbers do not have them
+// * The number of SSA uses is accurate or an over-estimate
+// * The local/global bit is properly set or an over-estimate
+// * The has phi use bit is properly set or an over-estimate


nit. the grammar was hard to read.
Suggested change
// * The has phi use bit is properly set or an over-estimate
// * The "has phi use" bit is properly set or an over-estimate

In general, it could benefit from a few more comments, especially "overview of the algorithm" kind of comments, IMO.
I'll be making a follow-on change where I use some of this info, so will handle those updates there.

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 14, 2022

ghost assigned AndyAyersMS Oct 14, 2022

SingleAccretion reviewed Oct 14, 2022
View reviewed changes

build-analysisBot mentioned this pull request Oct 15, 2022
Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009
Open
2 tasks

build-analysisBot mentioned this pull request Oct 15, 2022
Tracking Nuget 429s dotnet/arcade#10885
Closed
2 tasks

SingleAccretion reviewed Oct 15, 2022
View reviewed changes

AndyAyersMS mentioned this pull request Oct 17, 2022
Add IR support for modeling multiple SSA definitions as one store #76139
Merged

BruceForstall reviewed Oct 26, 2022
View reviewed changes

Comment threadsrc/coreclr/jit/compiler.h Outdated
Comment threadsrc/coreclr/jit/fgdiagnostic.cpp Outdated

AndyAyersMS force-pushed the TrackSameBlockSSADefs branch from 5c476e0 to a4dcf40Compare October 29, 2022 01:37

SingleAccretion reviewed Oct 29, 2022
View reviewed changes

revise per feedback
b517c63

runfoappBot mentioned this pull request Oct 31, 2022
Infra improvements for Helix #68176
Closed

BruceForstall approved these changes Oct 31, 2022
View reviewed changes

AndyAyersMS merged commit 5ec2e87 into dotnet:mainNov 1, 2022

ghost locked as resolved and limited conversation to collaborators Dec 1, 2022

		const bool isDef = (tree->gtFlags & GTF_VAR_DEF) != 0;
		const bool isUse = !isDef \|\| ((tree->gtFlags & GTF_VAR_USEASG) != 0);

	// * The has phi use bit is properly set or an over-estimate
	// * The "has phi use" bit is properly set or an over-estimate

Uh oh!

Conversation

AndyAyersMS commented Oct 14, 2022

Uh oh!

ghost commented Oct 14, 2022

Uh oh!

AndyAyersMS commented Oct 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SingleAccretionOct 15, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Oct 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Oct 14, 2022

Uh oh!

AndyAyersMS commented Oct 14, 2022

Uh oh!

AndyAyersMS commented Oct 14, 2022

Uh oh!

AndyAyersMS commented Oct 14, 2022

Uh oh!

azure-pipelinesBot commented Oct 14, 2022

Uh oh!

AndyAyersMS commented Oct 15, 2022

Uh oh!

AndyAyersMS commented Oct 15, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Oct 19, 2022

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AndyAyersMS commented Oct 29, 2022

Uh oh!

SingleAccretionOct 29, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SingleAccretionOct 29, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SingleAccretionOct 15, 2022•
edited
Loading

SingleAccretionOct 29, 2022•
edited
Loading

SingleAccretionOct 29, 2022•
edited
Loading

SingleAccretionOct 29, 2022•
edited
Loading