Change the SSA def node to ASG

[mikedn]

Currently the SSA definition is considered to be the LCL_VAR/LCL_FLD node that's on the LHS of the ASG node - DefLoc::m_tree points to it and not to the ASG node.

This is inconvenient and inefficient when chasing SSA defs because one needs to use gtGetParent to find the ASG node from which the RHS can then be obtained.

It also turns out to be at least misleading if not potentially incorrect - during rationalization stores are inserted at ASG's position, not at its LHS position. Normally the LHS immediately precedes the ASG node. In some cases there may be intervening nodes but they're not supposed to interfere with LHS. In theory. Good luck proving that :)

So it makes more sense to consider the ASG node to be the SSA definition point.

Of course, ASG should eventually be replaced by STORE_LCL_VAR/STORE_LCL_FLD. This can be considered a very small step forward in that direction since it avoids the need for gtGetParent.

[mikedn]

@AndyAyersMS Unless there's some exotic test failure this should be mostly done, except some code cleanup & comments. In any case, I don't see any reason not to use the ASG node as the SSA def point, IMO it should have been like this from the beginning.

[4creators]

It does not belong to this PR but would it be possible to clean up the naming of members by replacing m_asg, m_blk and similar identifiers and start using self-documenting identifiers i.e. m_Assignment. IMHO we are not that much restricted by identifier naming and IMO skipping all 3 letter verb or noun shortcuts in the code would help very much in improving code reading and understanding. Even m_block would not complain very much if renamed to m_BasicBlock or m_basicBlock 😃

[AndyAyersMS]

Changes look good overall.

Would recommend in places where you add new methods or alter signatures that you add the new-style header comments.

[AndyAyersMS]

Thanks, this looks like a definite improvement.

cc @dotnet/jit-contrib

[mikedn]

Would recommend in places where you add new methods or alter signatures that you add the new-style header comments.

Sure, I was just looking around to see if there's anything left to cleanup before doing that. For example, I need to remove the block parameter from RenameUse, it's not needed.

[mikedn]

I similar change should be done in liveness, it also assumes that the definition occurs at the LCL_VAR node and not at the ASG node. But that can be done in a separate PR as far as I can tell, the chance that there's an interfering use or def between an ASG and its LHS is very close to 0.

Also, I'm pretty sure that memory SSA is broken because it ignores definitions generated by HWINTRINSIC stores. Oh well, that would not be the first time it happens, I'll see if I can come up with an example that actually shows this to be a problem.

[mikedn]

And of course, this also makes the JIT a tiny bit faster in PIN - ~0.3%: https://1drv.ms/x/s!Av4baJYSo5pjgtMO1EpY1a1EvjYgYw?e=v1FlLg

Oh well, I'll chalk up one more to #15108. SsaBuilder looks like it was written in one night as a POC and then the next day it automagically became production code. Happens.

[BruceForstall]

@mikedn I presume you are ready to remove the "[WIP]" tag?

[BruceForstall]

Any diffs?

[BruceForstall]

Gonna kick off a bigger test run:

/azp run coreclr-outerloop

[BruceForstall]

/azp run coreclr-outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[BruceForstall]

Hmmm... I guess you can't have any other text in that comment. Who knew?

[mikedn]

Any diffs?

No FX diffs but looks like some Pri1 tests failed so perhaps there's something odd going on, I'll investigate.

[mikedn]

Yeah, restoring the asserts that I accidentally deleted from RangeCheck was a good move. The original code rejected definitions generated by indirect local stores due to it checking if the LHS node is a direct descendant of the ASG node. The new code failed to do that.

You'd think that it shouldn't matter, that the rest of the RangeCheck code would figure out that the LHS isn't something that it understands. But it turns out that it's oblivious to the LHS and even manages to follow defs through struct copies and field access:

N035 ( 32, 36) [003328] -A-XG-------                 |     |  \--*  COMMA     byref  <l:$261, c:$260>
N017 (  3,  4) [003316] -A--G---R---                 |     |     +--*  ASG       int    $35d
N016 (  1,  1) [003315] D------N----                 |     |     |  +--*  LCL_VAR   int    V115 tmp115      d:2 $35d
N015 (  3,  4) [002225] ------------                 |     |     |  \--*  LCL_FLD   int    V63 tmp63        u:2[+4] Fseq[q, val] (last use) $35d
N034 ( 29, 32) [003327] ---XG-------                 |     |     \--*  COMMA     byref  <l:$261, c:$260>
N021 (  8, 11) [003320] ---X--------                 |     |        +--*  ARR_BOUNDS_CHECK_Rng void   <l:$2ce, c:$2cd>
N018 (  1,  1) [003317] ------------                 |     |        |  +--*  LCL_VAR   int    V115 tmp115      u:2 $35d
N020 (  3,  3) [003319] ---X--------                 |     |        |  \--*  ARR_LENGTH int    <l:$35f, c:$35e>

With V63 definition being a struct copy:

N003 (  7,  5) [002237] -A------R---              *  ASG       struct (copy) $VN.Void
N002 (  3,  2) [002235] D------N----              +--*  LCL_VAR   struct V63 tmp63        d:2
N001 (  3,  2) [000233] -------N----              \--*  LCL_VAR   struct V17 tmp17        u:5 (last use) $310

and V17 definition being an indirect field assignment, where the checked build crashed:

N007 ( 13, 13) [002180] -A--G---R---              *  ASG       int    $VN.Void
N006 (  6,  7) [002179] *---G--N----              +--*  IND       int    $296
N005 (  3,  5) [002178] ------------              |  \--*  ADDR      byref  $25d
N004 (  3,  4) [002168] U------N----              |     \--*  LCL_FLD   struct V17 tmp17        ud:4->5[+4] Fseq[q] <l:$12d, c:$12e>
N003 (  6,  5) [003300] n-----------              \--*  IND       int    $296
N002 (  3,  3) [003299] ------------                 \--*  ADDR      byref  $25c
N001 (  3,  2) [002176] ------------                    \--*  LCL_VAR   struct V61 tmp61        u:2 (last use) $45

But it's not clear what happens if V17 definition was a direct field assignment. It looks like RangeCheck would have followed it despite having not really knowing if the assigned field is the same field that was used in the range check it tries to eliminate.

Meh, RangeCheck... I'll investigate this more separately.

[AndyAyersMS]

@dotnet/jit-contrib anyone else want to comment?

If not, I'll merge this in a day or two.

[BruceForstall]

/azp run coreclr-outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[maryamariyan]

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

1. In your coreclr repository clone, create patch by running git format-patch origin
2. In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

[AndyAyersMS]

@jashook any way to see the queue lengths for CI tests? Some of these tests are taking forever.

[BruceForstall]

@AndyAyersMS As far as I can tell, the jobs all passed, but there's some GitHub issue where it didn't get updated. Here's the AzDO link: https://dev.azure.com/dnceng/public/_build/results?buildId=415715&view=logs

[AndyAyersMS]

@BruceForstall thanks, am going to merge this.