JIT: overview of proposed local var reference count changes

[AndyAyersMS]

This issue proposes a series of changes to reduce the overhead and increase the fidelity of local variable reference counts in the jit. The main idea is to get rid of the current costly and buggy incremental count maintenance in favor of batch updates that are done just before accurate ref counts are needed.

See discussion in #8715 for background.

Expected impact is:

• 4-5% improvement in tier0 / minopts / debug throughput
• minimal impact to optimized jit throughput
• smaller frame sizes in some cases where reference counts are currently inflated
• removal of workarounds to artificially inflate reference counts to avoid asserts

The more accurate reference counts and weighted counts are likely to cause widespread codegen diffs. Hopefully these will mostly be improvements, but some regressions are certainly possible.

Proposed steps are:

• encapsulate accesses to lvRefCnt and lvRefCntWtd in an API, make backing fields private (Investigate preallocating space for ICollections/IIListProvider in SelectEnumerableIterator methods #18979)
• characterize when ref count access and update is invalid, and update API to flag invalid access (System.Linq.Expressions factories throw NullReferenceException with global fields and global methods #19068)
• classify invalid accesses and figure out strategies for removing them (Add XmlSchema and Xslt test coverage for Non-Windows platforms #19012)
• once all invalid accesses are removed, add appropriate asserts to catch invalid accesses (System.Linq.Expressions factories throw NullReferenceException with global fields and global methods #19068)
• figure out what actions under lvaMarkRefs are required for all compilations vs only needed when optimizing (WebUtility.HtmlDecode does not decode all HTML5 character entities #19103, JIT: refactor ref count computation into a reusable utility method coreclr#19240)
• figure out minopts / debug codegen approach (WebUtility.HtmlDecode does not decode all HTML5 character entities #19103)
• ensure we have an algorithm that can properly recompute counts from scratch (NullReferenceException when calling Vector<float>.CopyTo in Release mode #19240)
• recompute counts after lower (Failed to cross build for ARM32 #19325)
• change from incremental updates to batched updates (System.Data.SqlClient.sln contains stresstest project that does not exist #19345)
• decide how often counts need to be made current (or track if they've possibly gone stale) (System.Data.SqlClient.sln contains stresstest project that does not exist #19345)
• sort through codegen diffs resulting from more accurate ref counts and fix what can be fixed (System.Data.SqlClient.sln contains stresstest project that does not exist #19345)
• try and clean up the logic in lvaMarkRefs to split out ref counting from other activities

cc @dotnet/jit-contrib

category:implementation
theme:ir
skill-level:expert
cost:large

[AndyAyersMS]

Currently local ref counts become valid after lvaMarkLocalVars and are supposed to stay valid for the remainder of compilation (will update as I discover more)....

Here are some of the references / updates to ref counts that happen before that point:

• lvaPromoteStructVar -- ref count bump for promoted reg arg fields for "prolog" init count
• fgMarkImplicitByRefArgs -- zero ref counts for implicit by-ref parameters
• fgMarkAddressExposedLocals -- track ref counts for implicit by-ref parameters
• fgRetypeImplicitByRefArgs -- use ref counts to decide whether to promote implict by-ref parameters
• fgMakeOutgoingStructArgCopy -- use implicit by-ref parameter ref count to try and detect if the last use of the parameter is a call arg; if so try and avoid a copy
• fgGrabTempWithImplicitUse -- eg for the inline pinvoke frame var, hidden stub parameter
• lvaMarkLocalVars does some ref count manipulation before turning on "ref counts valid" mode

[mikedn]

4-5% improvement in tier0 / minopts / debug throughput

I'm curious how did you come up with that estimate :). Also, do we even need ref counts in minopts?

[AndyAyersMS]

Perf estimates are from #8715.

The improvement under MinOpts increases to about 4.7%.

I have a reasonably up to date version of this so can re-validate ... probably a good idea.

[AndyAyersMS]

Yeah, performance estimates still seem to hold up, eg PMI of Microsoft.CodeAnalysis.CSharp with minopts goes from 1334ms to 1275ms, around 4.5%, based on the just-updated prototype.

Actual benefit may be higher because the prototype still leaves a lot of ref counting cruft behind.

[AndyAyersMS]

Roughly speaking, the "invalid" ref count accesses in the current scheme (ref count reads, incs, decs, or sets before the jit officially starts ref counting) fall into categories:

• setting up implicit ref counts for locals that will never have explicit IR references (referenced in the prolog, say, or referenced via EH or GC)
• setting up transient implicit ref counts for locals whose references will appear eventually once constructs are lowered
• conveying information during morph for the implicit byref promotion / unpromotion
• conveying information during morph for optimization of last use struct args to avoid copies
• count manipulations that seem easily avoidable

The implicit cases can perhaps better be handled by new local var property bits, eg lvHasImplicitReference or lvHasTemporaryImplicitReference. If these bits are set then the lvRefCnt() should always return nonzero counts (likely just adding 1 to the explicit ref count result is sufficient) and likewise for the weighted version. The distinction between always implicit and transiently implicit may not matter as it might be OK to lump them together into one category. But if not, then at some point when the appropriate lowering happens the transient implicit bit can be turned off and ref counts established for the variable.

The conveying cases can perhaps be tolerated by using a different set of APIs or a special parameter to permit temporary (short-lifetime) use of ref counts for some subset of locals.

Struct last use copy avoidance is arguably something better handled by the work on first class structs as that should be more general, more capable, and less fragile.

[AndyAyersMS]

dotnet/coreclr#19012 should clear up the implicit reference cases and the avoidable cases.

For the other cases, one thought is to essentially make ref counts into something like a phased var, where the count fields are valid only for a certain time and certain purpose. The various APIs would then be extended with an enum describing the accessing phase, and this would be error-checked against the current phase. This could extend later to describe intervals when the counts are valid or potentially stale.

The error checking is a bit clunky with the current API shape as we either also need to pass in the compiler instance or fetch it from TLS. But perhaps the latter is acceptable....?

[briansull]

The error checking is a bit clunky with the current API shape as we either also need to pass in the compiler instance or fetch it from TLS. But perhaps the latter is acceptable....?

It is fine for use in Debug only code, but we should avoid using it for retail code

[AndyAyersMS]

It would be something like this:

inline unsigned short LclVarDsc::lvRefCnt(RefCountState state) const
{
    
#if defined(DEBUG)
    assert(state != RCS_INVALID);
    Compiler* compiler = JitTls::GetCompiler();
    assert(compiler->lvaRefCountState == state);
#endif
    
    if (lvImplicitlyReferenced && (m_lvRefCnt == 0))
    {
        return 1;
    }
    
    return m_lvRefCnt;
}

[CarolEidt]

conveying information during morph for optimization of last use struct args to avoid copies

This rings a bell, but I don't recall exactly what's being done. Can you point me to this?

Re: the error checking, I think it's fine to fetch the compiler instance from TLS for an assert.

[AndyAyersMS]

Look at fgMakeOutgoingStructArgCopy

[CarolEidt]

Thanks @AndyAyersMS - I now recall that some time ago when I was making the first round of 1st class struct changes, I deleted this line: https://github.com/dotnet/coreclr/blob/master/src/jit/morph.cpp#L5297 but it caused diffs, which I was trying to avoid and I wanted to analyze it further - so I created that "to do" and it's still there :-(