Tiered rejit work items #27147

briansull · 2019-10-11T17:45:28Z

Use info.compFullName as the input to create the JIT's MethodHash
Update JitOrder to print out MethodHash and PerfScore
Change eeGetMethodFullName to expand class and struct names for the argument types and the return type
fixed issue where bad edge weight were set in fgFoldConditional

Made flEdgeWeightMin and flEdgeWeightMax private fields
Added new method setNewEdgeWeights
Added support method eeGetArgClass

BruceForstall

Generally looks good to me.

src/jit/block.h

src/jit/ee_il_dll.hpp

src/jit/flowgraph.cpp

src/jit/block.h

briansull · 2019-10-23T01:01:15Z

@BruceForstall @dotnet/jit-contrib PTAL

BruceForstall · 2019-10-23T01:28:22Z

I don't think you should add the JitOrderParser to the coreclr repo; I think you should put it in the jitutils repo instead. The jitOrderParser.cs file also needs a license header, and the filenames there should probably be all lower case to avoid future Linux issues.

jashook · 2019-10-23T04:14:10Z

I don't think you should add the JitOrderParser to the coreclr repo; I think you should put it in the jitutils repo instead. The jitOrderParser.cs file also needs a license header, and the filenames there should probably be all lower case to avoid future Linux issues.

I agree

src/tools/JitOrderParser/jitOrderParser.cs

jashook · 2019-10-23T04:16:06Z

src/jit/compiler.cpp

- printf(" mdToken | | RGN | Count | EH | FRM | LOOP | NRM | IND | BBs | Cnt | Cnt | Cnt | Alloc | IL | HOT | COLD | method name \n");
- printf("---------+-----+------+----------+----+-----+------+-----+-----+-----+-----+-----+-----+---------+------+-------+-------+-----------\n");
- // 06001234 | PRF | HOT | 219 | EH | ebp | LOOP | 15 | 6 | 12 | 17 | 12 | 8 | 28 p2 | 145 | 211 | 123 | System.Example(int)
+ printf(" | Profiled | Method | Method has | calls | Num |LclV |AProp| CSE | Perf |bytes | %3s codesize| \n", Target::g_tgtCPUName);


What perfscore gets output on platforms that are missing full support like arm*?

The default PerfScore algorithm counts each instruction executed as 1 unit.

src/tools/JitOrderParser/JitOrderParser.csproj

src/tools/JitOrderParser/JitOrderParser.sln

Update JitOrder to print out MethodHash and PerfScore Change eeGetMethodFullName to expand class and struct names for the argument types and the return type Fixed issue where bad edge weight were set in fgFoldConditional Made flEdgeWeightMin and flEdgeWeightMax private fields Added new method setEdgeWeights Added support method eeGetArgClass Added source for Tool to parse JitOrder output and associate PerfScores

briansull · 2019-10-30T18:38:37Z

@BruceForstall @jashook @dotnet/jit-contrib PTAL
I have addressed the feedback from Bruce and Jarret

With this change we will get unique hashes for all methods that have different signatures.

AndyAyersMS · 2019-10-30T19:03:32Z

Seems like the edge weight changes are unrelated to the rest and should be a separate PR. I'd hoped that we could get rid of the min/max weight scheme as it's just confusing. Why bother having two weights? Which one is more trustworthy?

I wonder if the hash changes resolve #1957? Do we now get unique hashes for IL stubs?

There are a number of places already where we call getArgClass directly. I never really understood the point of the wrappers for jit interface calls but it seems like we should be consistent and either always directly call or always call via a wrapper.

briansull · 2019-10-30T23:51:02Z

I wonder if the hash changes resolve #1957? Do we now get unique hashes for IL stubs?

Yes, IL Stubs are uniquely identified with the new methodhash

briansull · 2019-10-30T23:54:32Z

I ran into an assert with edge weights when doing IBC training and optimization using a checked build.
This edge weight work was done to allow me to find and fix the bug.
I would like to get this work checked in, redesign is a separate issue here.

briansull · 2019-10-30T23:55:58Z

@AndyAyersMS if you really want I can checked these items in separately.

briansull · 2019-10-31T00:00:40Z

src/jit/morph.cpp

+ edge = fgGetPredForBlock(bUpdated->bbNext, bUpdated);
+ newMaxWeight = bUpdated->bbWeight;
+ newMinWeight = min(edge->edgeWeightMin(), newMaxWeight);
+ edge->setEdgeWeights(newMinWeight, newMaxWeight);


These assignments of flEdgeWeightMax could violate the requirements that min-weight <= maxWeight

AndyAyersMS · 2019-10-31T01:39:50Z

@AndyAyersMS if you really want I can checked these items in separately

I think it's a good practice not to mix unrelated (or seemingly unrelated) changes in one PR, but I'm not going to ask you to split this one up, as the rest of the changes are diagnostic.

We don't have a good mechanism for creating repros or regression tests for IBC related bugs. Seems like we ought to figure out how to do this.

briansull · 2019-10-31T17:47:01Z

We don't have a good mechanism for creating repros or regression tests for IBC related bugs. Seems like we ought to figure out how to do this.

I agree that we should have a better way to create regression repros in this area.
(Checking in the IBC data and running the IbcMerge tool are the required steps)
I believe that their is some rework going on in the profile area,
when that comes online we should have the ability to added regression testing.

briansull · 2019-10-31T17:48:27Z

@BruceForstall @jashook @dotnet/jit-contrib PTAL

This is ready for checkin

jashook

lgtm

BruceForstall · 2019-10-31T18:01:53Z

There are a number of places already where we call getArgClass directly. I never really understood the point of the wrappers for jit interface calls but it seems like we should be consistent and either always directly call or always call via a wrapper.

There's no need for a wrapper unless the wrapper does additional work. In many cases, we use wrappers so the altjit case can do something different.

BruceForstall reviewed Oct 18, 2019

View reviewed changes

src/jit/block.h Outdated Show resolved Hide resolved

src/jit/ee_il_dll.hpp Show resolved Hide resolved

src/jit/flowgraph.cpp Outdated Show resolved Hide resolved

src/jit/block.h Outdated Show resolved Hide resolved

briansull force-pushed the tiered-rejit-workitem branch from 8ef99c5 to 64c0ea9 Compare October 21, 2019 23:15

briansull changed the title ~~[WIP] Tiered rejit workitem~~ Tiered rejit work items Oct 23, 2019

jashook reviewed Oct 23, 2019

View reviewed changes

briansull added 2 commits October 23, 2019 10:47

Remove jitOrderParser.cs

9c0c056

briansull force-pushed the tiered-rejit-workitem branch from 9379b1b to 9c0c056 Compare October 23, 2019 17:57

sandreenko added the area-CodeGen label Oct 23, 2019

briansull commented Oct 31, 2019

View reviewed changes

jashook approved these changes Oct 31, 2019

View reviewed changes

BruceForstall approved these changes Oct 31, 2019

View reviewed changes

briansull merged commit c6697a0 into dotnet:master Oct 31, 2019

briansull deleted the tiered-rejit-workitem branch October 31, 2019 18:05

sandreenko mentioned this pull request Nov 14, 2019

Return compGetTieringName printing. #27873

Merged

AndyAyersMS mentioned this pull request Jan 15, 2020

JIT\Directed\tailcall fails at runtime with jitstress=2 dotnet/runtime#1752

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tiered rejit work items #27147

Tiered rejit work items #27147

briansull commented Oct 11, 2019 •

edited

Loading

BruceForstall left a comment

briansull commented Oct 23, 2019

BruceForstall commented Oct 23, 2019

jashook commented Oct 23, 2019

jashook Oct 23, 2019

briansull Oct 23, 2019

briansull commented Oct 30, 2019

AndyAyersMS commented Oct 30, 2019

briansull commented Oct 30, 2019

briansull commented Oct 30, 2019

briansull commented Oct 30, 2019

briansull Oct 31, 2019

AndyAyersMS commented Oct 31, 2019

briansull commented Oct 31, 2019 •

edited

Loading

briansull commented Oct 31, 2019

jashook left a comment

BruceForstall commented Oct 31, 2019

Tiered rejit work items #27147

Tiered rejit work items #27147

Conversation

briansull commented Oct 11, 2019 • edited Loading

BruceForstall left a comment

Choose a reason for hiding this comment

briansull commented Oct 23, 2019

BruceForstall commented Oct 23, 2019

jashook commented Oct 23, 2019

jashook Oct 23, 2019

Choose a reason for hiding this comment

briansull Oct 23, 2019

Choose a reason for hiding this comment

briansull commented Oct 30, 2019

AndyAyersMS commented Oct 30, 2019

briansull commented Oct 30, 2019

briansull commented Oct 30, 2019

briansull commented Oct 30, 2019

briansull Oct 31, 2019

Choose a reason for hiding this comment

AndyAyersMS commented Oct 31, 2019

briansull commented Oct 31, 2019 • edited Loading

briansull commented Oct 31, 2019

jashook left a comment

Choose a reason for hiding this comment

BruceForstall commented Oct 31, 2019

briansull commented Oct 11, 2019 •

edited

Loading

briansull commented Oct 31, 2019 •

edited

Loading