Delete `GT_ASG` #85871

SingleAccretion · 2023-05-06T16:41:47Z

After many years of existing and some weeks of removal work, GT_ASG, the last large IR quirk, becomes no more, giving way to the store nodes in the entirety of the compiler.

Closes #10873.

ghost · 2023-05-06T16:41:59Z

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

Issue Details

Measuring TP.

Author:	SingleAccretion
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `community-contribution`
Milestone:	-

SingleAccretion · 2023-05-09T18:16:43Z

Full TP diff from before morph to after import:

SingleAccretion · 2023-05-16T15:55:07Z

Diffs:

One regression in tests because a SIMD local does not get promoted due to the changed order of how structs are handled in the importer - cases with return buffers used to not go throw gtNewAssignNode, thus no "is used as a SIMD intrinsic" flag was set for them. Most of these cases were quirked away, but here it wasn't (because the call was wrapped in a RET_EXPR).
A correctness fix in tests where GTF_IND_VOLATILE is now not lost for a struct store.

Of course, there is also a very nice TP diff.

BruceForstall · 2023-05-17T00:50:51Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr outerloop, runtime-coreclr gcstress0x3-gcstress0xc

azure-pipelines · 2023-05-17T00:51:20Z

Azure Pipelines successfully started running 4 pipeline(s).

SingleAccretion · 2023-05-17T14:34:58Z

Stress results analysis:

Stress: clean modulo known failures.
Libraries stress: pre-existing op2.HasIconFlag() assertion failures, what looks like Test_EventSource_EtwManifestGeneration* tests failing in CI #48798 and a bunch of Quic connection timeouts on Linux ARM. The timeouts were not present in the first stress run, so seems unlikely they're related.
GC stress: a number of timeouts, look to be pre-existing.

src/coreclr/jit/promotiondecomposition.cpp

…Asg-The-End

src/coreclr/jit/promotion.cpp

…Asg-The-End

AndyAyersMS

I looked through everything and only have a few minor comment suggestions.

One broader nit is that we now use "value" and "data" somewhat interchangeably for what we used to sometimes call the RHS. I personally find "value" to be more semantically inspiring but don't feel like making a change here—assuming we decide we should—should hold up this PR.

Given the volume of the diffs it would be good to get a second reviewer. I would also like to hold off merging this until after the preview 5 snap tomorrow.

@jakobbotsch can you also do a review?

src/coreclr/jit/importer.cpp

AndyAyersMS · 2023-05-21T16:12:55Z

src/coreclr/jit/importer.cpp


-                int      aflags = isLoadAddress ? CORINFO_ACCESS_ADDRESS : CORINFO_ACCESS_GET;
-                GenTree* obj    = nullptr;
+                GenTreeFlags indirFlags = impPrefixFlagsToIndirFlags(prefixFlags);


These aren't used until ~170 lines later, can we move this down closer to the use?

In the store case (below) there are quite a few more uses, so if you want to leave things this way for symmetry then perhaps mark it as const so it is clearer nothing in between is modifying it?

The flags may get modified by the call impImportStaticFieldAddress, this happens when importing a static readonly object field. I can move both of these declarations closer to their uses if you think it would be better (the motivation for having this them this high up was indeed symmetry).

SingleAccretion · 2023-05-21T17:11:29Z

One broader nit is that we now use "value" and "data" somewhat interchangeably for what we used to sometimes call the RHS. I personally find "value" to be more semantically inspiring.

Agree on that, there will be another "comment fixes" PR and I can fix datas then.

src/coreclr/jit/gentree.cpp

jakobbotsch · 2023-05-22T14:32:39Z

src/coreclr/jit/lclmorph.cpp

+    void MoveNodeToEnd(GenTreeLclVarCommon* node)
    {
-        if (node->gtNext == nullptr)
+        if ((m_prevNode == node) || (node->gtNext == nullptr))


Is node->gtNext == nullptr a meaningful check if SequenceLocal no longer unconditionally clears it?

I had to recheck, but it turns out the answer is yes. The scenario is when we have half-consistent state during local morph with m_stmtModified == true and a new (bashed to) local address node inserted into the tree. Arguably, we should not be sequencing locals once m_stmtModified becomes true, but that is pre-existing behavior.

src/coreclr/jit/promotion.cpp

jakobbotsch

LGTM also, great work!

jakobbotsch · 2023-05-24T10:51:54Z

Thanks again! Awesome to have this in!

BruceForstall · 2023-06-12T18:05:47Z

@SingleAccretion Thanks again for all your fantastic work improving the JIT!

am11 · 2023-06-12T18:25:48Z

Is there an issue tracking #10873 (comment) (for the remainder of rationalizer phase)?

SingleAccretion · 2023-06-12T18:27:40Z

I don't think there is; moving rationalization upwards means LIRizing HIR, which I don't believe there are plans for at this point in time.

ghost added community-contribution Indicates that the PR has been added by a community member area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels May 6, 2023

SingleAccretion force-pushed the Op-Req-Asg-The-End branch from 676e5b7 to 017f98c Compare May 6, 2023 23:56

build-analysis bot mentioned this pull request May 7, 2023

Tracking issue for CI build timeouts #76454

Closed

SingleAccretion force-pushed the Op-Req-Asg-The-End branch from 017f98c to 004b6bb Compare May 7, 2023 16:24

SingleAccretion force-pushed the Op-Req-Asg-The-End branch 5 times, most recently from 863a694 to 77f7cb6 Compare May 16, 2023 00:03

build-analysis bot mentioned this pull request May 16, 2023

DataContractSerializerTests.DCS_MyPersonSurrogate_Stress failing in CI #35066

Open

SingleAccretion added 14 commits May 16, 2023 17:46

Physical promotion

c8817fb

Early liveness

9a4aaf5

Local morph

127ac25

Add internal blocks

0a23094

Object allocator

845e0df

Inlining

00e2af5

Indirect call transformer

4c12d5a

Patchpoint transformation

0e492d6

Instrumentation

a2dbe21

Importer: production

519ecb9

Importer: consumption

5fec7f0

Delete assignment rationalization

879ac84

Simple dead code removal

9b4170e

More code removal

42fc3e9

runfoapp bot mentioned this pull request May 16, 2023

Infra improvements for Helix #68176

Closed

SingleAccretion force-pushed the Op-Req-Asg-The-End branch from 77f7cb6 to 8810b14 Compare May 16, 2023 15:46

Simplify BasicBlock::FirstNonPhiDef

5520bf8

jakobbotsch reviewed May 19, 2023

View reviewed changes

src/coreclr/jit/promotiondecomposition.cpp Show resolved Hide resolved

SingleAccretion added 2 commits May 19, 2023 20:10

Merge branch 'main' of https://github.com/dotnet/runtime into Op-Req-…

dceff47

…Asg-The-End

Duplicate a comment

75a2cd9

jakobbotsch reviewed May 19, 2023

View reviewed changes

src/coreclr/jit/promotion.cpp Outdated Show resolved Hide resolved

SingleAccretion added 2 commits May 20, 2023 16:08

Merge branch 'main' of https://github.com/dotnet/runtime into Op-Req-…

f31cc2d

…Asg-The-End

Remove dead code

31a824f

AndyAyersMS approved these changes May 21, 2023

View reviewed changes

Fix naming and comments

8da4ca4

tannergooding mentioned this pull request May 21, 2023

Optimizer should try to reuse identical memory address values in register #86549

Open

jakobbotsch reviewed May 22, 2023

View reviewed changes

src/coreclr/jit/gentree.cpp Show resolved Hide resolved

jakobbotsch reviewed May 22, 2023

View reviewed changes

src/coreclr/jit/promotion.cpp Show resolved Hide resolved

jakobbotsch approved these changes May 22, 2023

View reviewed changes

jakobbotsch merged commit 18270a3 into dotnet:main May 24, 2023

SingleAccretion deleted the Op-Req-Asg-The-End branch May 24, 2023 17:41

SingleAccretion mentioned this pull request May 25, 2023

Remove more ASG terminlogy from the codebase #86760

Merged

JulieLeeMSFT mentioned this pull request Jul 5, 2023

What's new in .NET 8 Preview 6 [WIP] dotnet/core#8437

Closed

3 tasks

ghost locked as resolved and limited conversation to collaborators Jul 13, 2023

Delete GT_ASG #85871

Delete GT_ASG #85871

Uh oh!

Conversation

SingleAccretion commented May 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 6, 2023

Uh oh!

SingleAccretion commented May 9, 2023

Uh oh!

SingleAccretion commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BruceForstall commented May 17, 2023

Uh oh!

azure-pipelines bot commented May 17, 2023

Uh oh!

SingleAccretion commented May 17, 2023

Uh oh!

Uh oh!

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AndyAyersMS May 21, 2023

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS May 21, 2023

Choose a reason for hiding this comment

Uh oh!

SingleAccretion May 21, 2023

Choose a reason for hiding this comment

Uh oh!

SingleAccretion commented May 21, 2023

Uh oh!

Uh oh!

jakobbotsch May 22, 2023

Choose a reason for hiding this comment

Uh oh!

SingleAccretion May 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jakobbotsch left a comment

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented May 24, 2023

Uh oh!

BruceForstall commented Jun 12, 2023

Uh oh!

am11 commented Jun 12, 2023

Uh oh!

SingleAccretion commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Delete `GT_ASG` #85871

Delete `GT_ASG` #85871

SingleAccretion commented May 6, 2023 •

edited

Loading

SingleAccretion commented May 16, 2023 •

edited

Loading

SingleAccretion commented Jun 12, 2023 •

edited

Loading