Add IR support for modeling multiple SSA definitions as one store

[SingleAccretion]

Up until this change, it has been the case that an SSA ref had to have 1-to-1 relationship with the node it was attached to (with the exception of CanBeReplacedWithItsField structs). This was a problem for modeling assignments to promoted structs that are not decomposable, i. e. made by calls (multi-reg or not). The solution has been to exclude the promoted fields from SSA and VN, which is a CQ problem.

This change solves this by allowing for encoding the SSA number that would represent multiple field definitions at once. The code comment describing the format is quite nice, I will copy it here:

// Encapsulates the SSA info carried by local nodes. Most local nodes have simple 1-to-1
// relationships with their SSA refs. However, defs of promoted structs can represent
// many SSA defs at the same time, and we need to efficiently encode that.
//
class SsaNumInfo final
{
// This can be in one of four states:
// 1. Single SSA name: > RESERVED_SSA_NUM (0).
// 2. RESERVED_SSA_NUM (0)
// 3. "Inline composite name": packed SSA numbers of field locals (each could be RESERVED):
// [byte 3]: [top bit][ssa num 3] (7 bits)
// [byte 2]: [ssa num 2] (8 bits)
// [byte 1]: [compact encoding bit][ssa num 1] (7 bits)
// [byte 0]: [ssa num 0] (8 bits)
// We expect this encoding to cover the 99%+ case of composite names: locals with more
// than 127 defs, maximum for this encoding, are rare, and the current limit on the count
// of promoted fields is 4.
// 4. "Outlined composite name": index into the "composite SSA nums" table. The table itself
// will have the very simple format of N (the total number of fields / simple names) slots
// with full SSA numbers, starting at the encoded index. Notably, the table entries will
// include "empty" slots (for untracked fields), as we don't expect to use the table in
// the common case, and in the pathological cases, the space overhead should be mitigated
// by the cap on the number of tracked locals.
//

The rest of changes are fairly mechanical: adding support for "composite" definitions where necessary: SSA, VN, copy propagation.

While the encoding does support creating not just composite defs, but composite uses, I do not expect us to take advantage of this, due to the complexity tradeoff not being there. Fortunately, unlike with defs, "missing" a use is not a correctness problem.

No code diffs, small (< 0.1%) TP regression, with a bit more substantial memory regression (~0.2%).

What's up with the TP improvements on x86?

Apparently, DefinesLocal was particularly expensive there:

Base: 1475618025, Diff: 1472228001, -0.2297%
SsaBuilder::RenamePushDef : 1132213 : NA : 8.97% : +0.0767%
`Compiler::optCopyPropPushDef'::`2'::<lambda_1>::operator() : 1085980 : NA : 8.61% : +0.0736%
`Compiler::fgValueNumberLocalStore'::`2'::<lambda_1>::operator() : 720279 : NA : 5.71% : +0.0488%
Compiler::optAssertionGen : 443159 : +4.30% : 3.51% : +0.0300%
Compiler::fgAssignSetVarDef : 426327 : +89.46% : 3.38% : +0.0289%
Compiler::lvaLclExactSize : 275118 : +111.85% : 2.18% : +0.0186%
Compiler::fgValueNumberLocalStore : -308577 : -43.11% : 2.45% : -0.0209%
GenTree::IsPartialLclFld : -348430 : -95.72% : 2.76% : -0.0236%
Compiler::optBlockCopyProp : -471489 : -7.09% : 3.74% : -0.0320%
Compiler::GetSsaNumForLocalVarDef : -514808 : -100.00% : 4.08% : -0.0349%
Compiler::optCopyPropPushDef : -621491 : -47.61% : 4.93% : -0.0421%
Compiler::optIsSsaLocal : -669148 : -100.00% : 5.30% : -0.0453%
SsaBuilder::RenameDef : -699833 : -34.37% : 5.55% : -0.0474%
GenTree::DefinesLocal : -3752039 : -80.98% : 29.73% : -0.2543%

The benefit is we get to model multi-reg assignments and get rid of the CanBeReplacedWithItsField special case. Expected improvements are in the -9K range for the benchmarks collection on ARM64, which seems worth the TP cost.

The complexity cost is high, but not "terribly" high. I am open to the opinion we shouldn't pursue this altogether, though.

Closes#41242.

[ghost]

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

Issue Details

---

No code diffs, small (< 0.1%) TP regression, with a bit more substantial memory regression (~0.2%).

The benefit is we get to model multi-reg assignments and get rid of the CanBeReplacedWithItsField special case. Expected improvements are in the -9K range for the benchmarks collection on ARM64, which seems worth the TP cost.

The complexity cost is high, but not "terribly" high. I am open to the opinion we shouldn't pursue this altogether, though.

Closes #41242.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[SingleAccretion]

@dotnet/jit-contrib, @AndyAyersMS, @jakobbotsch

[JulieLeeMSFT]

@AndyAyersMS please review this community PR. cc @dotnet/jit-contrib.

[AndyAyersMS]

I skimmed through this and overall, it looks good. Will go back through in detail soon.

It will intersect with #77055 and #76706.

[AndyAyersMS]

Fortunately, unlike with defs, "missing" a use is not a correctness problem.

I am not sure I understand this -- soon enough we will want to make sure our SSA graphs are complete.

[AndyAyersMS]

Issues in the review notes can be handled as follow-ups.

Are we in position to relax some of this?

runtime/src/coreclr/jit/ssabuilder.cpp

Lines 1647 to 1661 in 6c949f5

	if (varDsc->lvIsStructField &&
	(m_pCompiler->lvaGetParentPromotionType(lclNum) != Compiler::PROMOTION_TYPE_INDEPENDENT))
	{
	// SSA must exclude struct fields that are not independent
	// - because we don't model the struct assignment properly when multiple fields can be assigned by one struct
	// assignment.
	// - SSA doesn't allow a single node to contain multiple SSA definitions.
	// - and PROMOTION_TYPE_DEPENDEDNT fields are never candidates for a register.
	//
	returnfalse;
	}
	elseif (varDsc->lvIsStructField && m_pCompiler->lvaGetDesc(varDsc->lvParentLcl)->lvIsMultiRegRet)
	{
	returnfalse;
	}

[AndyAyersMS]

Seems like this could also be handled as part of the "def" ssa num, via another variant of composite/outline encoding?

[SingleAccretion]

Probably, but it'd be quite complex I think, and slower for the common case in terms of instruction counts at least.

[SingleAccretion]

I am not sure I understand this

Well, it's exactly that -- currently, it is not a problem for us to not know all uses of SSA variables.

With #77055, that would of course change, which is somewhat unfortunate, because morph doesn't let us know whether dependent promotion became so because of a non-decomposed use (most often a local field), or because of a non-decomposed def, so the choice would become to either enhance morph to differentiate between the two, or do the accounting work necessary to support uses.

[SingleAccretion]

the accounting work necessary to support uses

To elaborate, this would involve renaming composite uses, and supporting cloning composite references (one can notice the current code doesn't handle this because it doesn't need to - SSA definitions cannot be cloned).

[SingleAccretion]

Are we in position to relax some of this?

Yes, removing that dependent promotion check is the primary motivation for the change. As-is, we could remove the check after this change is merged, with some light amount of related bug fixes.

With the support for use accounting, it would require more expansive changes / thought.

[AndyAyersMS]

I am not sure I understand this

Well, it's exactly that -- currently, it is not a problem for us to not know all uses of SSA variables.

With #77055, that would of course change, which is somewhat unfortunate, because morph doesn't let us know whether dependent promotion became so because of a non-decomposed use (most often a local field), or because of a non-decomposed def, so the choice would become to either enhance morph to differentiate between the two, or do the accounting work necessary to support uses.

Probably it would be enough in the short run to know that for this local/def there were no uses that got left out of SSA, and the checker could be likewise "enhanced" to not flag errors for such locals. I expect most of the benefit to accrue from simple cases where there is just a block-local lifetime.

Down the road I am interested in trying to rename some the SSA webs; I wonder if this would be a relatively cheap way to make LSRA's life easier.

[AndyAyersMS]

Let's take this as is and sort through the rest over time.

[AndyAyersMS]

Thanks again -- this is a very nice enhancement.

[sandreenko]

it is great to see such improvement!

[sandreenko]

do I miss something or is it always 0? What does it mean?

[SingleAccretion]

Oops, that is definitely a mistake, it should be (sizeof(int) * 8) / BITS_PER_SIMPLE_NUM, i. e. the maximum number of fields that can be encoded compactly.

Will be fixed in #77238.

[sandreenko]

is 127 a limit for number of fields for the local or for number of all locals in the method? I expect that if we have 200 locals we would have some SsaNum > 200 and the compact encoding won't work or am I wrong?

[SingleAccretion]

is 127 a limit for number of fields for the local or for number of all locals in the method?

It is the number of SSA definitions, per a field local. So, to not fit into this limit would require code like below:

s.FieldOne=1;
...s.FieldOne=2;
...// 125 more definitions
...s.FieldOne=128;

(Or complex flow, because of PHIs)

[sandreenko]

nit: < 0?

[SingleAccretion]

It would look a bit awkward because both 3 and 4 are < 0, so I left it out.

[sandreenko]

do we have 2 spare bits in byte2 and byte0?

[SingleAccretion]

Yep. Since 127 definitions is already a lot, I decided being more precise was not worth it.