Expand Virtual Call targets earlier in Morph and allow CSE of the indirections by briansull · Pull Request #47808 · dotnet/runtime

briansull · 2021-02-03T18:01:40Z

Use COMPlus_JitExpandCallsEarly=0 to disable and use old behavior

briansull · 2021-02-03T18:03:50Z

SuperPMI AsmDiffs indicate thatwe generally see improves in code size:

337 total files with Code Size differences (211 improved, 126 regressed), 84 unchanged.
337 total methods with Code Size differences (211 improved, 126 regressed), 84 unchanged.
7712 total files with Code Size differences (5710 improved, 2002 regressed), 896 unchanged.
7712 total methods with Code Size differences (5710 improved, 2002 regressed), 896 unchanged.
369 total files with Code Size differences (260 improved, 109 regressed), 49 unchanged.
369 total methods with Code Size differences (260 improved, 109 regressed), 49 unchanged.

Updated SuperPmi numbers:

Summary of Code Size diffs:
310 total files with Code Size differences (212 improved, 98 regressed), 85 unchanged.
310 total methods with Code Size differences (212 improved, 98 regressed), 85 unchanged.
Summary of Code Size diffs:
7237 total files with Code Size differences (5510 improved, 1727 regressed), 726 unchanged.
7237 total methods with Code Size differences (5510 improved, 1727 regressed), 726 unchanged.
Summary of Code Size diffs:
364 total files with Code Size differences (251 improved, 113 regressed), 49 unchanged.
364 total methods with Code Size differences (251 improved, 113 regressed), 49 unchanged.

AndyAyersMS

Any way you can think of to share the expansion logic with lower?

Note in a pure AOT compile (not sure how we know this) all the method pointer fetches can be marked invariant as there's no runtime backpatching.

src/coreclr/jit/morph.cpp

AndyAyersMS · 2021-02-03T19:16:11Z

src/coreclr/jit/morph.cpp

I realize this just mirrors what is over in LowerVirtualVtableCall, but I don't undertstand the If here -- there is no conditional code that follows.

I changed the comment to

// When isRelative is true we need to setup two temporary variables

The are two cases being covered here the 'isRealtive' case and the normal case.
Currently I have never seen 'isRelative' being true. I will add an assert and look into when it is used by the VM.

Not sure if we ever test these paths.

cc @jkotas

These paths were added by Samsung. There were only ever used by Tizen armel target.

It is set here:

runtime/src/coreclr/vm/jitinterface.cpp

Line 8830 in bf9f899

*isRelative = MethodTable::VTableIndir_t::isRelative ? 1 : 0;

@alpencolt Are you still using the relative pointers?

We use it in FNV (CoreCLR 3.1), but this mode doesn't work in .NET 6 now.
If we'll improve R2R performance relative pointers will not be used. Otherwise we'll have to fix FNV.

@jkotas Yes, But I wasn't able to see how MethodTable::VTableIndir_t::isRelative was ever set to non-zero

It is set to non-zero when FEATURE_NGEN_RELOCS_OPTIMIZATIONS is defined.

@jkotas Maybe it gets set in some tricky way that I couldn't figure out.
Anyway I will leave support in for this until we get a better understanding of what Samsung's plans are for this.

AndyAyersMS · 2021-02-03T19:17:30Z

src/coreclr/jit/morph.cpp

Would be nice to have a bit more descriptive name for these temps, eg "first-level vtable", "second-level vtable"

Is that what they mean? it wasn't clear to me that was their meanings.

Maybe the first one could be called the "indirection slot address", and the second one the "slot address"?

See eg:

runtime/src/coreclr/vm/methodtable.h

Lines 1399 to 1429 in 148bc08

//-------------------------------------------------------------------

// The VTABLE

//

// Rather than the traditional array of code pointers (or "slots") we use a two-level vtable in

// which slots for virtual methods live in chunks. Doing so allows the chunks to be shared among

// method tables (the most common example being between parent and child classes where the child

// does not override any method in the chunk). This yields substantial space savings at the fixed

// cost of one additional indirection for a virtual call.

//

// Note that none of this should be visible outside the implementation of MethodTable; all other

// code continues to refer to a virtual method via the traditional slot number. This is similar to

// how we refer to non-virtual methods as having a slot number despite having long ago moved their

// code pointers out of the vtable.

//

// Consider a class where GetNumVirtuals is 5 and (for the sake of the example) assume we break

// the vtable into chunks of size 3. The layout would be as follows:

//

// pMT chunk 1 chunk 2

// ------------------ ------------------ ------------------

// | | | M1() | | M4() |

// | fixed-size | ------------------ ------------------

// | portion of | | M2() | | M5() |

// | MethodTable | ------------------ ------------------

// | | | M3() |

// ------------------ ------------------

// | ptr to chunk 1 |

// ------------------

// | ptr to chunk 2 |

// ------------------

//

// We refer to "ptr to chunk 1" and "ptr to chunk 2" as "indirection slots."

I'd like to get rid of the unused isRelative code path,
I'm really not OK with changing these names here when I don't really understand the implementation
and I didn't see the implementation (or documentation of it) in the VM directory.

@AndyAyersMS
The comment in methodtable.h that you linked to is what we use when isRelative is false.

It results in three indirections

…expanded early during fgMorph Added COMPlus_JitExpandCallsEarly variable to enable virtual calls to be expanded early on a per method basis Set opts.compExpandCallsEarly to true when we are optimizing and have COMPlus_JitExpandCallsEarly enabled Update gtSetEvalOrder to also include the call->gtControlExpr Update morph to call fgExpandVirtualVtableCallTarget when we are expanding early Update lower to not call LowerVirtualVtableCall when we have already expanded it early Modify CheckTreeId to print the duplicated gtTreeID before it asserts. All tests are passing when using COMPLUS_JitExpandCallsEarly=* Expand the Virtual Call target after we morph the args Fix an inadvertent change in the GT_CALL weights

Use COMPlus_JitExpandCallsEarly=0 to disable and use old behavior

Added comment stating the the isRelative code path is never executed

briansull · 2021-02-09T21:18:58Z

I am investigating a few issues with 'Extra flags on tree' that I uncovered using superpmi.

briansull · 2021-02-12T07:06:28Z

I have addressed all of the issues that I found while running Super PMI with this change.
There were a few issues with keeping the gtFlags up to date when we have a gtControlExpr
And for tailcalls when we have a virtual call with an instance pointer that had a side-effect, the code gen was sub-optimal,
to what we would generate for a late expansion. This was probably more of a tailcall codegen issue, but I now check for the (rare) case and do a late expansion instead.

@AndyAyersMS PTAL

briansull · 2021-02-12T07:11:28Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2021-02-12T07:11:43Z

Azure Pipelines successfully started running 1 pipeline(s).

briansull · 2021-02-12T08:13:31Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2021-02-12T08:13:44Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS

Pointed out one small style issue...

AndyAyersMS · 2021-02-12T09:06:50Z

src/coreclr/jit/importer.cpp

+
+                // Should we expand virtual call targets early for this method?
+                //
+                if (opts.compExpandCallsEarly == true)


Suggested change

if (opts.compExpandCallsEarly == true)

if (opts.compExpandCallsEarly)

briansull · 2021-02-15T00:10:28Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2021-02-15T00:10:42Z

Azure Pipelines successfully started running 1 pipeline(s).

briansull · 2021-02-15T16:32:10Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2021-02-15T16:32:25Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 3, 2021

AndyAyersMS reviewed Feb 3, 2021

View reviewed changes

briansull mentioned this pull request Feb 8, 2021

JIT: allow CSE a hoisting of vtable lookups #46884

Closed

briansull linked an issue Feb 8, 2021 that may be closed by this pull request

JIT: allow CSE a hoisting of vtable lookups #46884

Closed

JulieLeeMSFT assigned briansull Feb 8, 2021

JulieLeeMSFT added this to the 6.0.0 milestone Feb 8, 2021

AndyAyersMS mentioned this pull request Feb 9, 2021

Dynamic PGO #43618

Closed

54 tasks

briansull added 3 commits February 9, 2021 10:06

Changed the default for Virtual calls to be expanded early in Morph

7410e53

Use COMPlus_JitExpandCallsEarly=0 to disable and use old behavior

Code Review feedback

da5b50f

Added comment stating the the isRelative code path is never executed

Fixes for propagating gtControlExpr->gtFlags

a6a4606

briansull force-pushed the cse-vtables branch from fbcac04 to a6a4606 Compare February 10, 2021 23:15

Fix a few code size regressions when we perform a tail call

4eb47c8

Tailcall lower fix

f0f7936

AndyAyersMS approved these changes Feb 12, 2021

View reviewed changes

briansull added 2 commits February 12, 2021 10:21

Code Review changes

d4e4c8a

Fixes for the TAILCALL_HELPER path for x86

6977170

Address the Arm/Linux failure

3413543

briansull merged commit 02b7358 into dotnet:master Feb 16, 2021

kunalspathak mentioned this pull request Feb 17, 2021

Assertion failed 'call->gtControlExpr == nullptr' in 'System.Text.Json.JsonParameterInfo1[Nullable1] #48403

Closed

JulieLeeMSFT mentioned this pull request Feb 24, 2021

What's new in .NET 6 Preview 2 dotnet/core#5889

Closed

ghost locked as resolved and limited conversation to collaborators Mar 18, 2021

	//-------------------------------------------------------------------
	// The VTABLE
	//
	// Rather than the traditional array of code pointers (or "slots") we use a two-level vtable in
	// which slots for virtual methods live in chunks. Doing so allows the chunks to be shared among
	// method tables (the most common example being between parent and child classes where the child
	// does not override any method in the chunk). This yields substantial space savings at the fixed
	// cost of one additional indirection for a virtual call.
	//
	// Note that none of this should be visible outside the implementation of MethodTable; all other
	// code continues to refer to a virtual method via the traditional slot number. This is similar to
	// how we refer to non-virtual methods as having a slot number despite having long ago moved their
	// code pointers out of the vtable.
	//
	// Consider a class where GetNumVirtuals is 5 and (for the sake of the example) assume we break
	// the vtable into chunks of size 3. The layout would be as follows:
	//
	// pMT chunk 1 chunk 2
	// ------------------ ------------------ ------------------
	// \| \| \| M1() \| \| M4() \|
	// \| fixed-size \| ------------------ ------------------
	// \| portion of \| \| M2() \| \| M5() \|
	// \| MethodTable \| ------------------ ------------------
	// \| \| \| M3() \|
	// ------------------ ------------------
	// \| ptr to chunk 1 \|
	// ------------------
	// \| ptr to chunk 2 \|
	// ------------------
	//
	// We refer to "ptr to chunk 1" and "ptr to chunk 2" as "indirection slots."

	if (opts.compExpandCallsEarly == true)
	if (opts.compExpandCallsEarly)

Conversation

briansull commented Feb 3, 2021

Uh oh!

briansull commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull commented Feb 9, 2021

Uh oh!

briansull commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

briansull commented Feb 12, 2021

Uh oh!

azure-pipelines bot commented Feb 12, 2021

Uh oh!

briansull commented Feb 12, 2021

Uh oh!

azure-pipelines bot commented Feb 12, 2021

Uh oh!

AndyAyersMS 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!

briansull commented Feb 15, 2021

Uh oh!

azure-pipelines bot commented Feb 15, 2021

Uh oh!

briansull commented Feb 15, 2021

Uh oh!

azure-pipelines bot commented Feb 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

briansull commented Feb 3, 2021 •

edited

Loading

briansull commented Feb 12, 2021 •

edited

Loading