JIT: Further clean up local definition handling

[jakobbotsch]

Move the node visitor to GenTreeCall and inline call definition flag setup into fgMorphCall. Use logical definitions for assertion killing and SSA memory definitions, deduplicating shared physical definitions.

Separate address-exposed local memory definitions from other memory modifications, sharing the SSA bookkeeping for exceptional successors.

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[jakobbotsch]

This is the source of the improvements. For example, for

               [000259] UA---------                            └──▌  STORE_LCL_FLD long  (P) V11 tmp6         [+8]
                                                                  ▌    ref    field V11._array (fldOffset=0x0) -> V28 tmp23        
                                                                  ▌    int    field V11._offset (fldOffset=0x8) -> V29 tmp24        
                                                                  ▌    int    field V11._count (fldOffset=0xc) -> V30 tmp25   

previously we would kill V11 which kills assertions about all of V11, V28, V29 and V30. Now using VisitLogicalLocalDefs kills only the assertions about V11, V29 and V30.

(Of course the precision is not the reason for the change, but to make this just work with GT_STORE_LCL_VARS)

[EgorBo]

@jakobbotsch not sure if it's a correctness issue, but does it mean a store into the padding doesn't kill the "this whole struct is ZeroObj" assertion?

[jakobbotsch]

Probably isn't a correctness issue yeah, but regardless, my other change to VisitLogicalLocalDefs will also mean we kill base local for that case.

Once old promotion is removed all of these edge cases will disappear naturally.

[jakobbotsch]

This was a bug introduced with async work. After fixing this earlier phases complained that the async calls weren't marked as GTF_ASG. I fixed that by adding the AsyncCallInfo::DefinedResumedIndicator to model that definition similarly to how we model retbuf definitions (they only become 'actual' definitions starting from local morph now).

[jakobbotsch]

Unnecessary, GTF_VAR_DEF is set by local morph when the retbuffer/async indicators are recognized as valid definitions that do not need address exposure.

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @EgorBo

Another few clean ups before the GT_STORE_LCL_VARS PR.

Diffs. Some minor improvements, I left a comment above where they're from and some other comments for context on the changes.

[EgorBo]

My AI reviewer flagged this as reachable:

This can still be reached with a local node. A  STORE_LCL_FLD  into a promoted struct that only touches padding (e.g.  { byte A; long B; } , 1-byte store at  +1 ) yields no logical defs.  anyDefs  stays false, and  RenameDef  (L453–L464) calls this with the store, so the assert fires. The old code handled local nodes here.

[EgorBo]

using System;
using System.Runtime.CompilerServices;

struct S {
    public byte A;
    public long B;
}

class Program {
    static void Main() => Console.WriteLine(Test(1, 2));

    [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
    static long Test(byte a, long b) {
        S s;
        s.A = a;
        s.B = b;
        Unsafe.Add(ref Unsafe.As<S, byte>(ref s), 1) = 42; // padding-only store
        return Unsafe.As<S, long>(ref s) + s.B;
    }
}

With the PR's JIT it fails with:

Assertion failed '!defNode->OperIsAnyLocal()' in 'Program:Test(byte,long):long'
during 'SSA: insert phis'  File: src\coreclr\jit\ssabuilder.cpp:540

[jakobbotsch]

Hmm, that's rather a bug in VisitLogicalLocalDefs. Let me fix that there.

[jakobbotsch]

I pushed a new commit. It has some TP regressions, but once old promotion is removed it can all be simplified a lot.