Add `JitEnregStats` complus. #58776

sandreenko · 2021-09-07T23:57:40Z

This PR adds a COMPlus/DOTNET_JitEnregStats to show additional information about our local enregistration.

Example output for aspnet.run.windows.x64.checked.mch:

Locals enregistration statistics:
total number of locals: 574832, number of enregistered: 345129, notEnreg: 229703, ratio: 0.60
total number of struct locals: 30650, number of enregistered: 369, notEnreg: 30281, ratio: 0.01
total number of primitive locals: 544182, number of enregistered: 344760, notEnreg: 199422, ratio: 0.63
m_addrExposed 34775, ratio: 0.15
m_notRegSizeStruct 639, ratio: 0.00
m_structArg 31, ratio: 0.00
m_blockOp 552, ratio: 0.00
m_localField 4798, ratio: 0.02
lvLiveInOutOfHndlr 1099, ratio: 0.00
lvDepField 1151, ratio: 0.01
lvNoRegVars 186363, ratio: 0.81
m_castTakedAddr 12, ratio: 0.00
m_swizzleArg 199, ratio: 0.00
m_blockOpRet 84, ratio: 0.00

Addr exposed details:
m_parentExposed 7910, ratio: 0.23
m_tooConservative 9008, ratio: 0.26
m_escapeAddress 17256, ratio: 0.50
m_copyFldByFld 593, ratio: 0.02
m_wideIndir 8, ratio: 0.00

I was surprised to see so many Tier0 (Min opts) methods in the collection but otherwise, it looks right (lvNoRegVars 186363, ratio: 0.81).

No diffs, but so some asserts in both base/diff spmi collection with GT_PHI

runtime/src/coreclr/jit/gentree.cpp

Lines 7808 to 7810 in 3fd30b0

    
           default: 
        
               NO_WAY("Cloning of node not supported"); 
        
               goto DONE;

, has somebody else seen this and do we have an issue for it?

ghost · 2021-09-07T23:57:47Z

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

Issue Details

null

Author:	sandreenko
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

sandreenko · 2021-09-07T23:59:09Z

/azp list

azure-pipelines · 2021-09-07T23:59:14Z

Commenter does not have sufficient privileges for PR 58776 in repo dotnet/runtime

JulieLeeMSFT · 2021-09-08T00:10:05Z

/azp list

azure-pipelines · 2021-09-08T00:10:10Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr jitstress runtime-coreclr jitstressregs runtime-coreclr jitstress2-jitstressregs runtime-coreclr gcstress0x3-gcstress0xc runtime-coreclr gcstress-extra runtime-coreclr r2r-extra runtime-coreclr jitstress-isas-x86 runtime-coreclr jitstress-isas-arm runtime-coreclr jitstressregs-x86 runtime-coreclr libraries-jitstressregs runtime-coreclr libraries-jitstress2-jitstressregs runtime-coreclr r2r runtime-coreclr runincontext runtime-coreclr crossgen2 runtime-libraries-coreclr outerloop runtime-libraries-coreclr outerloop-windows runtime-libraries-coreclr outerloop-linux runtime-libraries-coreclr outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-dev-innerloop runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono runtime-coreclr crossgen2-composite runtime-jit-experimental runtime-coreclr libraries-jitstress dotnet-linker-tests runtime-coreclr ilasm runtime-coreclr crossgen2-composite gcstress runtime-libraries-mono outerloop runtime-staging runtime-coreclr pgo runtime-coreclr libraries-pgo coreclr-gc-regions

JulieLeeMSFT · 2021-09-08T00:11:48Z

@sandreenko what pipleline do you want to run? I will run it.

sandreenko · 2021-09-08T03:08:56Z

@sandreenko what pipleline do you want to run? I will run it.

thank you Julie, I was just testing how /azp will react.
Btw is there an option to create a draft PR that does not do this "Tagging subscribers to this area" (like a very raw change that the author wants to keep unnoticed for some time).

sandreenko · 2021-09-08T03:17:33Z

Hmm, I can't call @dotnet/jit-contrib even if I am inside it looks like.

PTAL @BruceForstall @kunalspathak @AndyAyersMS

JulieLeeMSFT · 2021-09-08T03:19:48Z

@sandreenko what pipleline do you want to run? I will run it.

thank you Julie, I was just testing how /azp will react.
Btw is there an option to create a draft PR that does not do this "Tagging subscribers to this area" (like a very raw change that the author wants to keep unnoticed for some time).

@eiriktsarpalis do you have access to the bot to not tag owners for draft PRs?

eiriktsarpalis · 2021-09-08T12:35:51Z

do you have access to the bot to not tag owners for draft PRs?

There doesn't seem to be an option to exclude draft PRs in FabricBot. We could perhaps ask the FB team to implement this /cc @danmoseley

kunalspathak

Thanks for doing this. Added few comments.

kunalspathak · 2021-09-08T05:54:40Z

src/coreclr/jit/jitconfigvalues.h

@@ -382,6 +382,10 @@ CONFIG_INTEGER(JitMinOptsTrackGCrefs, W("JitMinOptsTrackGCrefs"), JitMinOptsTrac
 // TODO-Cleanup: need to make 'MEASURE_MEM_ALLOC' well-defined here at all times.
 CONFIG_INTEGER(DisplayMemStats, W("JitMemStats"), 0) // Display JIT memory usage statistics

+#if defined(DEBUG)
+CONFIG_INTEGER(JitEnregStats, W("JitEnregStats"), 0) // Display JIT enregistration statistics


nit: Have this under #ifdef DEBUG near LSRA stats for easy discoverability.

kunalspathak · 2021-09-08T06:00:16Z

src/coreclr/jit/gschecks.cpp

        shadowVarDsc->lvDoNotEnregister = varDsc->lvDoNotEnregister;
 #ifdef DEBUG
-        shadowVarDsc->lvVMNeedsStackAddr = varDsc->lvVMNeedsStackAddr;


why we don't need to set this anymore?

These were the debug fields to explain why we don't enregister, now they are replaced with

DoNotEnregisterReason m_doNotEnregReason; AddressExposedReason m_addrExposedReason;

kunalspathak · 2021-09-08T16:28:23Z

src/coreclr/jit/lclvars.cpp

+    LclVarDsc* varDsc = &lvaTable[varNum];
+    if (varDsc->lvDoNotEnregister == 1)
+    {
+        return;


Is it reasonable to add assert(varDsc->m_doNotEnregReason == reason)?

Or if varDsc was set to DoNotRegister by some other phase, then also set the reason here?

I was thinking to do DoNotEnregisterReason enum to be a set like some other enums:

#define MAKE_DNRREASON(bit) (1ULL << (bit)) AddrExposed= = MAKE_DNRREASON( 0), DontEnregStructs= = MAKE_DNRREASON( 1),

but because one reason is often causing others for the purpose of this dump it is clearer to keep only the first reason and not all of them for each var.

So we record only the first reason why something can't be put into a register and if different reasons come we ignore it.

kunalspathak · 2021-09-08T16:35:55Z

src/coreclr/jit/morph.cpp

                newVarDsc->lvDoNotEnregister       = varDsc->lvDoNotEnregister;
                newVarDsc->lvLiveInOutOfHndlr      = varDsc->lvLiveInOutOfHndlr;
                newVarDsc->lvSingleDef             = varDsc->lvSingleDef;
                newVarDsc->lvSingleDefRegCandidate = varDsc->lvSingleDefRegCandidate;
                newVarDsc->lvSpillAtSingleDef      = varDsc->lvSpillAtSingleDef;
 #ifdef DEBUG
-                newVarDsc->lvLclBlockOpAddr            = varDsc->lvLclBlockOpAddr;


Why we don't need to clone these fields?

answered above, they are deleted.

kunalspathak · 2021-09-08T16:39:53Z

src/coreclr/jit/fgbasic.cpp

-        lvaTable[info.compThisArg].lvHasILStoreOp = false;
+        noway_assert(arg0varDsc->lvVerTypeInfo.IsThisPtr());
+        thisVarDsc->lvVerTypeInfo.ClearThisPtr();
+        // Note that here we don't clear `m_doNotEnregReason` and it stays


Why don't we clear address exposed reason?

I saw only 1 method that was using this logic in benchmark collection so I did not dig deep here.

In fgAdjustForAddressExposedOrWrittenThis we are creating a clone for this and replace other uses of the original this with the clone, so we assume that now the reason why this was marked as address exposed is no longer valid and there could be no other reasons why it could be address exposed.
So after this transformation, the clone is address exposed and the original - not.

However, it is questionable if we can clear lvDoNotEnregister here as well because we don't have information if AddrExposed was the only reason why we can't enregister. So we can imagine a situation where we clear this flag and enregister the original this when we should not, for example, because local enreg is disabled or something like that.

src/coreclr/jit/compiler.cpp

src/coreclr/jit/compiler.h

kunalspathak · 2021-09-08T16:54:13Z

src/coreclr/jit/compiler.cpp

+        return;
+    }
+
+    if (m_addrExposed != 0)


It will be better readable if we have a macro something like this:

#define PRINT_STATS(stat, total) \ if (stat != 0) \ { \ fprintf(fout, #stat "%d, ratio: %.2f\n", stat, (float)stat / total); \ } \ PRINT_STATS(m_addrExposed, notEnreg) PRINT_STATS(m_notRegSizeStruct , notEnreg) ... PRINT_STATS(m_copyFldByFld, m_addrExposed)

BruceForstall

Looks like a nice cleanup, with useful stats

BruceForstall · 2021-09-08T21:11:35Z

src/coreclr/jit/compiler.h

@@ -676,6 +707,49 @@ class LclVarDsc
        return lvIsMultiRegArg || lvIsMultiRegRet;
    }

+private:
+    bool m_addrExposed : 1; // The address of this variable is "exposed" -- passed as an argument, stored in a


This isn't going to bitwise pack with anything, since there aren't adjacent bool bit ops. Is there a reason this can't stay as before? (unsigned char :1) I'm not sure if you can change public/private visibility within a packing set; something to check.

Also, which m_ is a "new standard", it seems like since nothing else in this class is defined with that naming convention, we should stick with what's already here and use the lv previx like everything else.

Also, which m_ is a "new standard", it seems like since nothing else in this class is defined with that naming convention, we should stick with what's already here and use the lv previx like everything else.

the other fields in this struct are mostly public so for public lv prefix makes sense (if somebody is using grep to find them and not IDE tools) but for private fields I don't see any value in having them.

This isn't going to bitwise pack with anything, since there aren't adjacent bool bit ops. Is there a reason this can't stay as before? (unsigned char :1)

fixed, but with bool instead of char if you don't mind.

fixed, but with bool instead of char if you don't mind.

I thought that unsigned char :1 doesn't pack with bool :1 due to the rules of C bitfield packing, or maybe MSVC++ behavior. Can you check? It might be safest to use unsigned char just to ensure the greatest likelihood of packing across different compilers.

https://godbolt.org/ shows that both clang and MSVC don't have problems with usnigned int a:1; bool b:1 packing.

BruceForstall · 2021-09-08T21:15:51Z

src/coreclr/jit/jit.h

@@ -440,6 +440,7 @@ class GlobalJitOptions
 #define MEASURE_MEM_ALLOC 1 // Collect memory allocation stats.
 #define LOOP_HOIST_STATS 1  // Collect loop hoisting stats.
 #define TRACK_LSRA_STATS 1  // Collect LSRA stats
+#define TRACK_ENREG_STATS 1 // Collect enregistration stats


Set

#define TRACK_ENREG_STATS 0

in the #else clause, like the other cases?

I was considering it but the contract here is that you then will be able to set #define TRACK_ENREG_STATS 1 and get data in the release. It is not the case here, in order to make TRACK_ENREG_STATS 1 work in the release I would need to replace many DEBUGARG with something like

#if defined(DEBUG) || (TRACK_ENREG_STATS == 1) #define DEBUG_OR_TACK_ENREG_STATS(arg) , arg #endif

and it looked to me as overkill, especially because for this mode we don't expect different results from the release and checked like for others.

BruceForstall · 2021-09-08T21:19:01Z

src/coreclr/jit/lclvars.cpp

+    if (varDsc->lvDoNotEnregister == 1)
+    {
+        return;
+    }


How about adding something to JitDump if the new reason is not the same as the existing reason?

not sure about it, for example, lets say a var is marked as DoNotEnreg 2 times because of addrExposed and 2 times because of lclFld, the first reason is addrExposed, what would you prefer to see it the dump:
1.

should not be enregistered because: addrExposed should not be enregistered also because: lclFld// a new reason should not be enregistered because: addrExposed // the first reason called again should not be enregistered also because: lclFld// a new reason, don't have information that we already had lclFld before.

should not be enregistered because: addrExposed should not be enregistered also because: lclFld should not be enregistered also because: addrExposed should not be enregistered also because: lclFld

should not be enregistered because: addrExposed should not be enregistered also because: lclFld should not be enregistered also because: lclFld

I was thinking of (3). I think it's better for the lclFld lines (in this example) to be duplicated than for there to be no output at all. Unless you tell me the number of duplicated lines will be large (>20), at which point maybe there's some other issue (like, why do we call do-not-enreg so much?)

BruceForstall · 2021-09-08T21:22:10Z

src/coreclr/jit/lower.cpp

            const GenTreeLclVarCommon* lclAddr = node->AsLclVarCommon();
-            comp->lvaSetVarDoNotEnregister(lclAddr->GetLclNum() DEBUGARG(Compiler::DNER_BlockOp));
+            const LclVarDsc*           varDsc  = comp->lvaGetDesc(lclAddr);
+            if (!varDsc->lvDoNotEnregister)


Shouldn't this always call lvaSetVarDoNotEnregister and it can early-return if already set (but also possibly JitDump if already set for a different reason)

not in this case in my opinion, if everything goes well it would be replaced with assert(varDsc->lvDoNotEnregister) soon because this code is a patch for some locals that are slipping morph analysis nowadays.

not in this case in my opinion, if everything goes well it would be replaced with assert(varDsc->lvDoNotEnregister) soon because this code is a patch for some locals that are slipping morph analysis nowadays.

src/coreclr/jit/compiler.h

src/coreclr/jit/compiler.cpp

kunalspathak · 2021-09-09T17:55:28Z

@sandreenko - Do you mind rebasing your branch and fixing formatting issues and then it should be ready to go.

sandreenko · 2021-09-09T19:20:59Z

@sandreenko - Do you mind rebasing your branch and fixing formatting issues and then it should be ready to go.

done, also sorted switches and prints according to the enum order.

AndyAyersMS · 2021-09-09T19:38:42Z

Since lvNoRegVars is not a particularly interesting reason (at least in conjunction with Tier0), perhaps you should just do the normalized breakdown of cases where this doesn't appear...?

sandreenko · 2021-09-09T23:03:24Z

Since lvNoRegVars is not a particularly interesting reason (at least in conjunction with Tier0), perhaps you should just do the normalized breakdown of cases where this doesn't appear...?

I can change that if you think it will be better. I think sometimes the presence of lvNoRegVars can indicate that we fall back to minOpts where we did not expect it, for example, when we do a replay of crossgen2 collection we don't expect any of them.

kunalspathak · 2021-09-13T15:12:58Z

Thank you @sandreenko !

ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 7, 2021

dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed community-contribution Indicates that the PR has been added by a community member labels Sep 7, 2021

sandreenko marked this pull request as ready for review September 8, 2021 03:03

kunalspathak reviewed Sep 8, 2021

View reviewed changes

BruceForstall requested changes Sep 8, 2021

View reviewed changes

kunalspathak approved these changes Sep 9, 2021

View reviewed changes

Sergey and others added 5 commits September 9, 2021 10:03

Add JitEnregStats complus.

3cff1db

review response.

6f586e0

response review 2.

51319bc

add a space

4e344dd

fix printing/sorting

cfa2d3a

BruceForstall approved these changes Sep 9, 2021

View reviewed changes

sandreenko force-pushed the setDoNotEnregReasonForPR branch from ea5e954 to cfa2d3a Compare September 9, 2021 17:58

sandreenko added 2 commits September 9, 2021 11:07

fix merge conflicts

5963b2c

format

b4024ef

kunalspathak merged commit b2fbf3d into dotnet:main Sep 13, 2021

AndyAyersMS mentioned this pull request Sep 17, 2021

Refactor subrange assertion generation and propagation #55186

Merged

ghost locked as resolved and limited conversation to collaborators Nov 3, 2021

Add JitEnregStats complus. #58776

Add JitEnregStats complus. #58776

Conversation

sandreenko commented Sep 7, 2021 • edited Loading

ghost commented Sep 7, 2021

sandreenko commented Sep 7, 2021

azure-pipelines bot commented Sep 7, 2021

JulieLeeMSFT commented Sep 8, 2021

azure-pipelines bot commented Sep 8, 2021

JulieLeeMSFT commented Sep 8, 2021

sandreenko commented Sep 8, 2021

sandreenko commented Sep 8, 2021

JulieLeeMSFT commented Sep 8, 2021

eiriktsarpalis commented Sep 8, 2021 • edited Loading

kunalspathak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalspathak commented Sep 9, 2021

sandreenko commented Sep 9, 2021

AndyAyersMS commented Sep 9, 2021

sandreenko commented Sep 9, 2021

kunalspathak commented Sep 13, 2021

Add `JitEnregStats` complus. #58776

Add `JitEnregStats` complus. #58776

sandreenko commented Sep 7, 2021 •

edited

Loading

eiriktsarpalis commented Sep 8, 2021 •

edited

Loading