Eliminate GenTreeRegVar and GT_REG_VAR and RegVar #18317

juliusfriedman · 2018-06-05T23:34:13Z

Complete. Please let me know if there is anything else.

ASCIIEncoding.cs - Updated syntax to use nameof and default and lambda, updated null checks to use "yoda" conditions, checked against false rather than performing NOT, performed parameter checks in parallel where possible, removed use of charCount in GetCharCount and used count directly. Used int.MaxValue vs 0x7fffffff in GetMaxCharCount and GetMaxByteCount, skipped declaring fallbackResult and used the value directly. DecoderFallback.cs - Indicated Span API usefulness, marked ThrowLastBytesRecursive static. (Encoding.cs could also mark similar methods static) EncoderFallback.cs - Updated syntax to use lamba and default, InternalGetNextChar now uses switch, InternalFallback checks against false directly instead of using NOT, marked ThrowLastCharRecursive static. EncoderReplacementFallback.cs - Updated syntax to use lambda, default, is and yoda conditions. EncoderReplacementFallbackBuffer.Fallback uses shift vs Divide.

…g "chars", noted with comment, removed commented code. Encoding.cs - noted hardcoded string "chars". ASCIIEncoding.cs - Added referenced to System.Numerics,System.Runtime.CompilerServices, Internal.Runtime.CompilerServices. Used references to implement TryGetAsciiChars, TryEncodeAsciiCharsToBytes and optimize GetByteCount, GetBytes, GetCharCount, GetChars using those implementations, noted Allocations used in Fallback.

Removed GPL code link for comparison. Added 2 other resources. Fixed order of checks (yoda conditionals) moved calculation of charCount. noted byteCount was already checked to be > 0. Passed pointers by reference. moved declaration of charEnd. moved calculation of charCount.

codegencommon.cpp - Removed branches and asserts for GT_REG_VAR. congenlinear.cpp - removed restoreRegVar and checks for GT_REG_VAR. compiler.cpp - removed cases and checks for GT_REG_VAR. compiler.h - removed GT_REG_VAR case. compiler.hpp - removed GT_REG_VAR case. flowgraph.cpp - removed check for GT_REG_VAR. gcinfo.cpp - removed GT_REG_VAR case. gentree.cpp - removed assert for size of GT_REG_VAR, removed cases for GT_REG_VAR. gentree.h - updated comments from removing GT_REG_VAR, removed checks for GT_REG_VAR, removed asserts for GT_REG_VAR, eliminated GenTreeRegVar. gtlist.h - removed GTNODE for REG_VAR. gtstructs.h - removed use of GT_REG_VAR and RegVar liveness.cpp - removed check for GT_REG_VAR. lower.cpp - removed check in assert for GT_REG_VAR. regset.cpp - removed assert for GT_REG_VAR. valuenum.cpp - removed case for GT_REG_VAR. morph.cpp - removed check for GT_REG_VAR. rationalize.cpp - removed cases for GT_REG_VAR.

Fixed formatting errors.

This reverts commit 1fc647d.

codegencommon.cpp - Removed branches and asserts for GT_REG_VAR. congenlinear.cpp - removed restoreRegVar and checks for GT_REG_VAR. compiler.cpp - removed cases and checks for GT_REG_VAR. compiler.h - removed GT_REG_VAR case. compiler.hpp - removed GT_REG_VAR case. flowgraph.cpp - removed check for GT_REG_VAR. gcinfo.cpp - removed GT_REG_VAR case. gentree.cpp - removed assert for size of GT_REG_VAR, removed cases for GT_REG_VAR. gentree.h - updated comments from removing GT_REG_VAR, removed checks for GT_REG_VAR, removed asserts for GT_REG_VAR, eliminated GenTreeRegVar. gtlist.h - removed GTNODE for REG_VAR. gtstructs.h - removed use of GT_REG_VAR and RegVar liveness.cpp - removed check for GT_REG_VAR. lower.cpp - removed check in assert for GT_REG_VAR. regset.cpp - removed assert for GT_REG_VAR. valuenum.cpp - removed case for GT_REG_VAR. morph.cpp - removed check for GT_REG_VAR. rationalize.cpp - removed cases for GT_REG_VAR.

Fixed formatting errors.

This reverts commit 5bdc8a6.

This reverts commit e5505c4.

This reverts commit dbc8208.

codegencommon.cpp - Removed branches and asserts for GT_REG_VAR. congenlinear.cpp - removed restoreRegVar and checks for GT_REG_VAR. compiler.cpp - removed cases and checks for GT_REG_VAR. compiler.h - removed GT_REG_VAR case. compiler.hpp - removed GT_REG_VAR case. flowgraph.cpp - removed check for GT_REG_VAR. gcinfo.cpp - removed GT_REG_VAR case. gentree.cpp - removed assert for size of GT_REG_VAR, removed cases for GT_REG_VAR. gentree.h - updated comments from removing GT_REG_VAR, removed checks for GT_REG_VAR, removed asserts for GT_REG_VAR, eliminated GenTreeRegVar. gtlist.h - removed GTNODE for REG_VAR. gtstructs.h - removed use of GT_REG_VAR and RegVar liveness.cpp - removed check for GT_REG_VAR. lower.cpp - removed check in assert for GT_REG_VAR. regset.cpp - removed assert for GT_REG_VAR. valuenum.cpp - removed case for GT_REG_VAR. morph.cpp - removed check for GT_REG_VAR. rationalize.cpp - removed cases for GT_REG_VAR.

Fixed formatting errors.

This reverts commit 6959a3c.

This reverts commit 7aa42ca.

This reverts commit dbc8208.

This reverts commit 6959a3c.

This reverts commit fc47321.

This reverts commit 5bdc8a6.

This reverts commit e5505c4.

gentree.h - fixed comment rationalize.cpp - removed code left over from removal of GT_REG_VAR case

CarolEidt · 2018-06-07T00:33:28Z

LGTM overall - thanks!! It will be great to see these gone.
There are a couple of places that now need asserts, but otherwise this is ready to go.
If you were inclined, you could squash the commits (or we can do it on merge).

CarolEidt

A couple of asserts are needed.

CarolEidt · 2018-06-07T00:26:33Z

src/jit/compiler.cpp

-                    {
-                        chars += printf("[REG_BIRTH]");
-                    }
+                    chars += printf("[REG_BIRTH]");


This flag isn't really in use, but #18196 will take care of eliminating it.

This entire if needs to go away. The flag only applies to GT_REG_VAR nodes so attempting to print it for other nodes will result in misleading dumps.

Based on @mikedn 's comment I will remove it now.

CarolEidt · 2018-06-07T00:28:00Z

src/jit/gcinfo.cpp

        unsigned lclNum = 0;
        if (tgtAddr->gtOper == GT_LCL_VAR)
        {
            lclNum = tgtAddr->gtLclVar.gtLclNum;


Since the case below is now gone, this should have an assert(tgtAddr->gtOper == GT_LCL_VAR);

Looks like not even an assert is needed since it's obvious that tgtAddr is GT_LCL_VAR already.

Similar to the previous case, you should replace these 5 lines with:

unsigned lclNum = tgtAddr->AsLclVar()->GetLclNum();

CarolEidt · 2018-06-07T00:28:22Z

src/jit/codegencommon.cpp

            wbKind          = CWBKind_OtherByRefLocal; // Unclassified local variable.
            unsigned lclNum = 0;
            if (lcl->gtOper == GT_LCL_VAR)
                lclNum = lcl->gtLclVarCommon.gtLclNum;


Since the case below is now gone, this should have an assert(tgtAddr->gtOper == GT_LCL_VAR);

Please clarify, the code below was an else to what is already such a check albeit a different variable. I think you mean to insert one at 3254? Just want to be correct.

Instead of an explicit assert, gtLclVarCommon should be replaced with AsLclVar that provides the assert for free. The whole thing should be unsigned lclNum = lcl->AsLclVar()->GetLclNum();

As @mikedn has suggested, these 3 lines should just be replaced with:

unsigned lclNum = lcl->AsLclVar()->GetLclNum();

The AsLclVar() will do the assert that this is a GT_LCL_VAR node, and this should be the only kind of node we see here now.

(In general, we're moving torward the AsXX() methods instead of the gtXX tags.)

CarolEidt · 2018-06-07T00:30:22Z

src/jit/gentree.h

        ClearRegOptional();
    }

    bool IsRegVarDeath() const


These two methods are not in use, bug again, #18196 can eliminate them.

mikedn · 2018-06-07T04:20:13Z

src/jit/gentree.h

 //         GTF_VAR_DEF && !GTF_VAR_USEASG
-#define GTF_REG_BIRTH       0x04000000 // GT_REG_VAR -- enregistered variable born here
-#define GTF_VAR_DEATH       0x02000000 // GT_LCL_VAR, GT_REG_VAR -- variable dies here (last use)
+#define GTF_REG_BIRTH       0x04000000 // GT_LCL_VAR, -- enregistered variable born here


@CarolEidt Is this comment change valid? I don't think GTF_REG_BIRTH is used at all now.

Right - I don't think it's correct, but I think it's reasonable to eliminate it with #18196

mikedn · 2018-06-07T04:24:40Z

src/jit/gentree.h

    bool IsRegVarDeath() const
    {
-        assert(OperGet() == GT_REG_VAR);
        return (gtFlags & GTF_VAR_DEATH) ? true : false;


The correct change in such a case would be to either delete the entire function or replace the assert with unreached(). But since these functions will probably end up being delete as @CarolEidt says, you can leave this as is.

juliusfriedman · 2018-06-07T19:50:02Z

If 18196 has decided to eliminate them I can do that also, I would like to get this sorted out and make a separate PR for that unless you guys want me to combine them.

Let me know how you want me to proceed so it's easier for you guys or what needs to be done further at this point.

…G_VAR.

BruceForstall · 2018-09-05T21:18:32Z

@juliusfriedman Are you still planning to work on this?

juliusfriedman · 2018-12-01T22:25:22Z

Sorry for the delay, just noticed... if you need something else done outside of the commit just lmk.

mikedn · 2018-12-02T04:52:07Z

There's some feedback in the review comments, a couple of ifs that are now redundant and should be removed. Also, it would be nice to cleanup the commit history, there are a bunch of unrelated commits.

CarolEidt

@juliusfriedman - sorry for the delay. There are a couple of changes remaining, and it would be great if you could squash the commits (though I believe we can do that at merge time).

CarolEidt · 2018-12-03T23:51:07Z

src/jit/codegencommon.cpp

            wbKind          = CWBKind_OtherByRefLocal; // Unclassified local variable.
            unsigned lclNum = 0;
            if (lcl->gtOper == GT_LCL_VAR)
                lclNum = lcl->gtLclVarCommon.gtLclNum;


As @mikedn has suggested, these 3 lines should just be replaced with:

unsigned lclNum = lcl->AsLclVar()->GetLclNum();

The AsLclVar() will do the assert that this is a GT_LCL_VAR node, and this should be the only kind of node we see here now.

(In general, we're moving torward the AsXX() methods instead of the gtXX tags.)

CarolEidt · 2018-12-04T00:00:43Z

src/jit/gcinfo.cpp

        unsigned lclNum = 0;
        if (tgtAddr->gtOper == GT_LCL_VAR)
        {
            lclNum = tgtAddr->gtLclVar.gtLclNum;


Similar to the previous case, you should replace these 5 lines with:

unsigned lclNum = tgtAddr->AsLclVar()->GetLclNum();

juliusfriedman

I have applied the suggested fixed in codegencommon.cpp, gcinfo.cpp and gentree.h. When removing GTF_VAR_BIRTH and GTF_VAR_DEATH I see uses of them in the genRegCopy function of codegenarmarch.cpp. It seems the branch can now be removed however that would be resolved in another commit for which addresses 18196. At the current time I will shelve the changes removing gentree.h or undo them and just apply the unreached call within IsRegVarDeath and IsRegVarBirth (gentree.h) and if desires I can make another commit to remove the branching which relies on the GTF_VAR_BIRTH and GTF_VAR_DEATH defines from the other files in another commit; or you guys are welcome to do so at merge time. Please let me know if that is acceptable and if I missed anything.

juliusfriedman · 2018-06-07T19:58:18Z

src/jit/codegencommon.cpp

            wbKind          = CWBKind_OtherByRefLocal; // Unclassified local variable.
            unsigned lclNum = 0;
            if (lcl->gtOper == GT_LCL_VAR)
                lclNum = lcl->gtLclVarCommon.gtLclNum;


Please clarify, the code below was an else to what is already such a check albeit a different variable. I think you mean to insert one at 3254? Just want to be correct.

…um()` instead of inline ceremony. gcinfo.cpp - gcWriteBarrierFromTargetAddress; used `lcl->AsLclVar()->GetLclNum()` instead of inline ceremony. gentree.h IsRegVarDeath and IsRegVarBirth now call unreached to prepare for removal.

juliusfriedman · 2018-12-08T19:18:18Z

juliusfriedman@09e9b4b

juliusfriedman · 2018-12-08T19:24:46Z

@CarolEidt, @BruceForstall,@mikedn, @danmosemsft
Please let me know if anything else is required;

CarolEidt · 2018-12-11T00:50:55Z

test Ubuntu16.04 arm64 Cross Checked no_tiered_compilation_innerloop Build and Test

CarolEidt

LGTM - thanks for doing this - and for your patience!

Issue #18201 / Hackathon

AndyAyersMS · 2018-12-15T00:28:08Z

Did we look at diffs for this? Benchview reports a regression on x64 ubuntu:

CarolEidt · 2018-12-15T01:44:45Z

Did we look at diffs for this?

No, we didn't. The changes seemed to all be benign/no-ops, but apparently they were not.

mikedn · 2018-12-15T08:43:52Z

Are we sure that the regression was caused by this change? I tried to revert the change and I can't find any diffs, at least when using the Linux AltJit. Diffed framework, diffed benchmark, tried PMI, nothing.

AndyAyersMS · 2018-12-15T16:11:59Z

No, not sure. Thanks for digging in.

I took a wider look (past 1000 results) and this benchmark has a slower level that can remain stable for long periods of time.

So it appears this is "normal" behavior and there's no regression.

(colors swapped from above, blue is now Ubuntu, green, Windows)

Issue dotnet/coreclr#18201 / Hackathon Commit migrated from dotnet/coreclr@e1a5d0f

Julius Richard Friedman added 30 commits June 1, 2018 18:48

Merge branch 'master' of https://github.com/juliusfriedman/coreclr

a60c654

Issue #18201 / Hackathon

d60421f

Fixed formatting errors.

Revert "Hackathon / Issue 18208"

ad7dc29

This reverts commit 1fc647d.

Issue #18201 / Hackathon

5bdc8a6

Fixed formatting errors.

Revert "Issue #18201 / Hackathon"

cf97315

This reverts commit 5bdc8a6.

Revert "Issue #18201 / Hackathon"

82ba7cf

This reverts commit e5505c4.

Reverts commits related to 18208

dbb2eda

Merge branch 'master' of https://github.com/juliusfriedman/coreclr

947c0b5

Reverts commits related to 18208

7aa42ca

Revert "Optimize Encoding.ASCII #18208"

6959a3c

This reverts commit dbc8208.

Issue #18201 / Hackathon

3843359

Fixed formatting errors.

Revert "Revert "Optimize Encoding.ASCII #18208""

fc47321

This reverts commit 6959a3c.

Revert "Reverts commits related to 18208"

394c8fb

This reverts commit 7aa42ca.

Revert "Optimize Encoding.ASCII #18208"

921d3e6

This reverts commit dbc8208.

Revert "Revert "Optimize Encoding.ASCII #18208""

acd0717

This reverts commit 6959a3c.

Revert "Revert "Revert "Optimize Encoding.ASCII #18208"""

efbe402

This reverts commit fc47321.

Revert "Issue #18201 / Hackathon"

6909f20

This reverts commit 5bdc8a6.

Revert "Issue #18201 / Hackathon"

1d71e30

This reverts commit e5505c4.

Reverted changes from 18208

d7ca3af

Merge branch 'master' of https://github.com/juliusfriedman/coreclr

673f98c

Fixed whitespace changes.

1d0bea3

More whitespace changes reverted

8ae183b

Last whitespace changes!?!?

868a0d9

Julius Richard Friedman added 6 commits June 5, 2018 17:57

!!!

d4362f3

!

eaf3a91

!!

dce2ea8

Eliminate GenTreeRegVar and GT_REG_VAR

68b60cd

gentree.h - fixed comment rationalize.cpp - removed code left over from removal of GT_REG_VAR case

Applied suggested formatting.

bfd763f

Removed a whitespace manually.

425b4fe

CarolEidt suggested changes Jun 7, 2018

View reviewed changes

mikedn reviewed Jun 7, 2018

View reviewed changes

Removed print statement which was only relevent for now removed GT_RE…

7d516f6

…G_VAR.

CarolEidt suggested changes Dec 4, 2018

View reviewed changes

juliusfriedman commented Dec 8, 2018

View reviewed changes

CarolEidt approved these changes Dec 11, 2018

View reviewed changes

CarolEidt merged commit e1a5d0f into dotnet:master Dec 11, 2018

briansull mentioned this pull request Dec 12, 2018

Fix for PMI assert: 'vnWx != NoVN' #21512

Merged

morganbr pushed a commit that referenced this pull request Dec 14, 2018

Eliminate GenTreeRegVar and GT_REG_VAR and RegVar (#18317)

b10774f

Issue #18201 / Hackathon

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Eliminate GenTreeRegVar and GT_REG_VAR and RegVar (dotnet/coreclr#18317)

53b476b

Issue dotnet/coreclr#18201 / Hackathon Commit migrated from dotnet/coreclr@e1a5d0f

Eliminate GenTreeRegVar and GT_REG_VAR and RegVar #18317

Eliminate GenTreeRegVar and GT_REG_VAR and RegVar #18317

Uh oh!

Conversation

juliusfriedman commented Jun 5, 2018

Uh oh!

CarolEidt commented Jun 7, 2018

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusfriedman Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusfriedman commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BruceForstall commented Sep 5, 2018

Uh oh!

juliusfriedman commented Dec 1, 2018

Uh oh!

mikedn commented Dec 2, 2018

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusfriedman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusfriedman commented Dec 8, 2018

Uh oh!

juliusfriedman commented Dec 8, 2018

Uh oh!

CarolEidt commented Dec 11, 2018

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Dec 15, 2018

Uh oh!

CarolEidt commented Dec 15, 2018

Uh oh!

mikedn commented Dec 15, 2018

Uh oh!

AndyAyersMS commented Dec 15, 2018

juliusfriedman Jun 7, 2018 •

edited

Loading

juliusfriedman commented Jun 7, 2018 •

edited

Loading