Transform indirect calls to direct ones in the importer #85197

MichalPetryka · 2023-04-22T14:01:11Z

Imports indirect calls as direct ones when the target method is known.

Only handles addresses from ldftn as the VM has no way to verify pointers from static readonly fields and crashes on invalid ones.

The helpers currently contain a small dead path that I'll soon use when extending the code to also handle delegates.

Opening as a draft so that the code can be reviewed while I finish the tests.

Fixes #44610

ghost · 2023-04-22T14:01:23Z

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

Issue Details

Imports indirect calls as direct ones when the target method is known.

Only handles addresses from ldftn as the VM has no way to verify pointers from static readonly fields and crashes on invalid ones.

The helpers currently contain a small dead path that I'll soon use when extending the code to also handle delegates.

Opening as a draft so that the code can be reviewed while I finish the tests.

Fixes #44610

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

AndyAyersMS · 2023-04-25T18:19:18Z

Some thoughts:

I would suggest splitting off the enhanced single-def changes as a separate PR. Ideally this would be a no diff change.
The single-def changes (if extended to cover more optimizations) is potentially quite useful on its own; function pointers are pretty rare but other invariant things (say constants) are not.
The backwards search for the single def is going to become problematic for throughput; consider instead tracking the def node directly or in a side table. Note cross-block maps will need to be invalidated in certain somewhat rare circumstances (reimportation for example).
Or, track which blocks have defs of which locals, so you can avoid searching in cases where the search will fail
Or, limit how far back you can search for the def, and limit the how much chaining you allow (hopping from one local to another).

Imports indirect calls as direct ones when the target method is known. Only handles addresses from ldftn as the VM has no way to verify pointers from static readonly fields and crashes on invalid ones. The helpers currently contain a small dead path that I'll soon use when extending the code to also handle delegates. Opening as a draft so that the code can be reviewed while I finish the tests. Fixes dotnet#44610

MichalPetryka · 2023-05-03T21:43:33Z

I'll be unable to respond to reviews for a few weeks, I've asked @thatbakamono to apply any feedback though.

AndyAyersMS · 2023-05-05T18:30:51Z

src/coreclr/jit/importer.cpp

+    };
+
+    GenTree*    valueNode = findValue(impStmtList);
+    BasicBlock* bb        = fgFirstBB;


I'm still ambivalent about this non-local searching.

Seems like we could have cases where we are able to do this transformation in Tier0-instr but not in a subsequent OSR, because OSR doesn't have the same fgFirstBB.

I suppose one fix is not to do this transformation in Tier0-instr (though removing the need to have a method probe is appealing)... but only if we can then always do the same in a subsequent rejit.

AndyAyersMS · 2023-05-05T18:32:13Z

src/coreclr/jit/importercalls.cpp

-    /*-------------------------------------------------------------------------
-     * First create the call node
-     */
+    // run transformations when instrumenting to not pollute PGO data


I'm not sure what you mean by this?

Are you saying if can optimize calli->call here when instrumenting we won't need a method probe? See my note above about whether this is an effective strategy given OSR.

AndyAyersMS · 2023-05-05T18:33:01Z

src/coreclr/jit/importercalls.cpp

+            GenTree* fptr = impStackTop().val;
+            if (fptr->OperIs(GT_LCL_VAR))
+            {
+                JITDUMP("impImportCall trying to import calli as call - trying to substitute LCL_VAR\n");


Would be nice to know in the dump which local var...

AndyAyersMS · 2023-05-05T18:35:58Z

src/coreclr/jit/importer.cpp

+
+    if (valueNode != nullptr && valueNode->OperIs(GT_LCL_VAR))
+    {
+        return impGetNodeFromLocal(valueNode);


Seems like this could get tripped up if there is a chain of local assignments where one of them is initialized later than it is used, eg

t0 = t1; t1 = <fnptr>; calli t0;

The chained search should start from where the previous search left off.

AndyAyersMS · 2023-05-05T18:40:02Z

src/coreclr/jit/importercalls.cpp

+        CORINFO_CLASS_HANDLE targetClass = info.compCompHnd->getMethodClass(replacementMethod);
+        info.compCompHnd->getMethodSig(replacementMethod, &methodSig, targetClass);
+
+        if (methodSig.hasImplicitThis() && targetThis == TYP_UNDEF)


Isn't targetThis always TYP_UNDEF at this point?

AndyAyersMS · 2023-05-05T18:43:42Z

src/coreclr/jit/importercalls.cpp

+    CORINFO_METHOD_HANDLE replacementMethod       = nullptr;
+    GenTree*              newThis                 = nullptr;
+    var_types             oldThis                 = TYP_UNDEF;
+    var_types             targetThis              = TYP_UNDEF;


I would rename these to be a bit more descriptive, eg oldThisTyp.

Also move the declaration/definition down closer to where they're used.

AndyAyersMS · 2023-05-05T18:44:26Z

src/coreclr/jit/importer.cpp

+        }
+    }
+
+    if (valueNode != nullptr && valueNode->OperIs(GT_LCL_VAR))


Add a jit dump here to track the progress of chaining back through other locals.

AndyAyersMS · 2023-05-05T18:49:31Z

src/coreclr/jit/importercalls.cpp

+                                   var_types         sourceThis,
+                                   var_types         targetThis)
+{
+    assert(sourceSig->hasImplicitThis() || sourceThis == TYP_UNDEF);


Seems like sourceThis is always TYP_UNDEF and targetThis is never TYP_VOID?

I can't tell if you are trying to future-proof this method and have it handle cases it currently won't be asked to handle, or if this is a vestige of some earlier version.

Since there is currently just one caller perhaps scope this down to just the cases you'll actually see, and assert if anything else comes along?

AndyAyersMS · 2023-05-05T18:51:33Z

src/coreclr/jit/importercalls.cpp

+//
+bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig,
+                                   CORINFO_SIG_INFO* targetSig,
+                                   var_types         sourceThis,


As above, sourceThisTyp and targetThisType?

Would be good to settle on one pair of adjectives to describe the two options here: one of original/new, old/net, source/target.

AndyAyersMS · 2023-05-05T18:53:57Z

src/coreclr/jit/importercalls.cpp

@@ -1622,6 +1708,138 @@ GenTree* Compiler::impFixupCallStructReturn(GenTreeCall* call, CORINFO_CLASS_HAN
 #endif // FEATURE_MULTIREG_RET
 }

+//-----------------------------------------------------------------------------------
+//  impCanSubstituteSig: Checks whether it's safe to replace a call with another one.


I wonder if we're better off implementing this entirely on the VM side of the interface; there's quite a bit of back and forth across the interface here, and the VM may already have something like this that you can just call (though maybe not for "jit sigs")... ?

ghost · 2023-06-04T23:01:51Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

AndyAyersMS · 2023-06-05T19:47:52Z

@MichalPetryka plans to work on this soon.

ghost · 2023-07-05T23:02:07Z

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 22, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 22, 2023

MichalPetryka force-pushed the indirect-inline branch from 34909e2 to 34e4195 Compare April 23, 2023 01:16

MihuBot mentioned this pull request Apr 23, 2023

[MichalPetryka] Transform indirect calls to direct ones in the importer MihuBot/runtime-utils#37

Closed

build-analysis bot mentioned this pull request Apr 23, 2023

IOException running NuGet-Migrations during tests in dotnet CLI first run #80619

Closed

runfoapp bot mentioned this pull request Apr 24, 2023

Long Running Test: Interop/MonoAPI/MonoMono/PInvokeDetach/PInvokeDetach.sh #73040

Closed

build-analysis bot mentioned this pull request Apr 25, 2023

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

2 tasks

MichalPetryka force-pushed the indirect-inline branch from 9ad26ef to bf44b09 Compare April 25, 2023 18:52

This was referenced Apr 25, 2023

Fix totalILArgs for explicithis #85347

Merged

Reduce calli address spilling #85349

Merged

This was referenced Apr 25, 2023

Tracking issue for CI build timeouts #76454

Closed

Could not load file or assembly 'Microsoft.CodeAnalysis.NetAnalyzers #84995

Closed

MichalPetryka mentioned this pull request Apr 26, 2023

Set lvSingleDef for non TYP_REF locals #85398

Merged

MichalPetryka added 14 commits May 1, 2023 20:09

Format, add tests for UCO

ee58f66

Fix warning

699cf4a

Spill unmanaged pointers

da5831d

Try to reduce diffs

3e9f88b

Revert change

5d626a1

Fix assert

4903e3a

Fix typo

7e9cdb4

Fix the this handling logic

e081790

Add IL tests, fix build

9c6c2c2

Fix warning

548e88a

Fetch target type

ea96e22

Reformat, add tests

3e1f208

Add pinvoke and no assign tests

050ec5b

MichalPetryka force-pushed the indirect-inline branch from bf44b09 to 050ec5b Compare May 1, 2023 20:06

MichalPetryka added 4 commits May 1, 2023 22:33

Update CMakeLists.txt

c06cdf0

Update IndirectNative.cpp

b9fa699

Update IndirectNative.cpp

ce759c9

Update IndirectNative.cpp

a19d9b1

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

Failures in System.Net.Mail.Tests.SmtpClientTest tests #85637

Closed

MichalPetryka mentioned this pull request May 2, 2023

Inliner: don't give up on virtual calls if we can clarify their exact targets after inlining #85209

Open

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

System.IO.Tests.RandomAccess_NoBuffering.ReadUsingSingleBuffer timing out #85659

Closed

AndyAyersMS reviewed May 5, 2023

View reviewed changes

ghost closed this Jun 4, 2023

AndyAyersMS reopened this Jun 5, 2023

ghost closed this Jul 5, 2023

ghost locked as resolved and limited conversation to collaborators Aug 5, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transform indirect calls to direct ones in the importer #85197

Transform indirect calls to direct ones in the importer #85197

MichalPetryka commented Apr 22, 2023

ghost commented Apr 22, 2023

AndyAyersMS commented Apr 25, 2023

MichalPetryka commented May 3, 2023

AndyAyersMS May 5, 2023

AndyAyersMS May 5, 2023

AndyAyersMS May 5, 2023

AndyAyersMS May 5, 2023

AndyAyersMS May 5, 2023

AndyAyersMS May 5, 2023

AndyAyersMS May 5, 2023

AndyAyersMS May 5, 2023

AndyAyersMS May 5, 2023

AndyAyersMS May 5, 2023

ghost commented Jun 4, 2023

AndyAyersMS commented Jun 5, 2023

ghost commented Jul 5, 2023

Transform indirect calls to direct ones in the importer #85197

Transform indirect calls to direct ones in the importer #85197

Conversation

MichalPetryka commented Apr 22, 2023

ghost commented Apr 22, 2023

AndyAyersMS commented Apr 25, 2023

MichalPetryka commented May 3, 2023

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

ghost commented Jun 4, 2023

AndyAyersMS commented Jun 5, 2023

ghost commented Jul 5, 2023