Fix GC hole with STOREIND of LCL_VAR_ADDR/LCL_FLD_ADDR

[BruceForstall]

We were updating the GC liveness of the ADDR node, but
genUpdateLife() expects to get the parent node, so no
liveness was ever updated.

There were 4 SPMI GC info diffs in the libraries, all
related to uses of System.Collections.Immutable.ImmutableArray
where we struct promote fields who are themselves single-element
gc ref structs that are kept on the stack and not in registers.
In all cases, the liveness of the stack local was not reflected
in codegen's GC sets, but it was reflected in the emitter's GC
sets, so it was marked as a GC lifetime. However, that lifetime
would get cut short if we hit a call site before the last use,
as calls (sometimes) carry the full set of live variables across
the call. So, variables not in this set (including the
"accidental" emitter-created GC lifetimes here) would get killed,
leaving a hole between the intermediate call and actual stack
local last use.

Fixes #45557

[BruceForstall]

See #45557 for more detailed IL + code analysis.

Additional work:

• See if this problem exists in 5.0 (presumably yes) and 3.1
• Try to create a reduce regression test
• Check other platforms for similar code

[BruceForstall]

cc @dotnet/jit-contrib

[BruceForstall]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BruceForstall]

It seems like the same change should be made in emitInsLoadInd(), but when I make that change, I don't see any diffs, so I don't really want to touch it.

[AndyAyersMS]

Should we assert over in UpdateLifeVar if we see addresses?

Will be interesting to determine whether this is a long-standing issue that somehow just surfaced now. From what I can tell the emitter/codegen parts haven't changed in a few years.

[BruceForstall]

Should we assert over in UpdateLifeVar if we see addresses?

This code in TreeLifeUpdater<ForCodeGen>::UpdateLife() filters out the addresses so they never get to UpdateLifeVar:

    if (!tree->OperIsNonPhiLocal() && compiler->fgIsIndirOfAddrOfLocal(tree) == nullptr)
    {
        return;
    }

so an assert wouldn't have helped in this case; not sure it would help in any other case.

Will be interesting to determine whether this is a long-standing issue that somehow just surfaced now. From what I can tell the emitter/codegen parts haven't changed in a few years.

Yes, these lines of code have been there a long time. My guess is that we never saw STOREIND with ref types until recently (maybe some of the struct work?), as they would have been block ops or STORE_OBJ instead.

[AndyAyersMS]

an assert wouldn't have helped in this case; not sure it would help in any other case.

How unfortunate. Seems like we have a fairly fragile contract here...

[sandreenko]

LGTM, thanks for the detailed analysis.

[AndyAyersMS]

Did you determine if this was a regression in 5.0?

[BruceForstall]

Did you determine if this was a regression in 5.0?

Yes, this looks like a regression in 5.0, apparently due to keeping struct types around instead of "unwrapping" or bashing the type to the single-element struct ref type.