JIT: Split out call importation into new importercalls.cpp #76792

jakobbotsch · 2022-10-09T11:25:14Z

No code changes yet, purely moving the code.

ghost · 2022-10-09T11:25:30Z

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

Issue Details

No code changes yes, purely moving code.

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

EgorBo · 2022-10-09T13:15:13Z

we already have importer_vectorization.cpp should we have importer_call.cpp ?

jakobbotsch · 2022-10-09T13:24:45Z

we already have importer_vectorization.cpp should we have importer_call.cpp ?

Most of the JIT files do not use underscores as separators so I'm not really sure. Seems like importer_vectorization is the odd one out.

jakobbotsch · 2022-10-09T13:31:39Z

Maybe we should rename importer_vectorization.cpp -> stringintrinsics.cpp to make it consistent with hwintrinsics.cpp?

EgorBo · 2022-10-09T13:32:24Z

we already have importer_vectorization.cpp should we have importer_call.cpp ?

Most of the JIT files do not use underscores as separators so I'm not really sure. Seems like importer_vectorization is the odd one out.

Feel free to fix that but currently it looks inconsistent. I'd prefer %phase name%%what it does% -- similar to LLVM: https://github.com/llvm/llvm-project/tree/main/llvm/lib/Transforms/InstCombine InstCombine phase splitted to InstCombineCalls, InstCombineCasts, etc

EgorBo · 2022-10-09T13:34:02Z

Maybe we should rename importer_vectorization.cpp -> stringintrinsics.cpp to make it consistent with hwintrinsics.cpp?

I have a batch of changes for it support bytes - not sure it can be called stringintrinsics after that.

EgorBo · 2022-10-09T13:36:40Z

And honestly, I hate hwintrinsics.cpp its name doesn't tell if it's Importer-phase related code or codegen or lower 🙂

jakobbotsch · 2022-10-09T13:42:04Z

How about I just rename importer_vectorization.cpp -> importervectorization.cpp, importcall.cpp -> importercalls.cpp and register_arg_convention -> registerargconvention?

EgorBo · 2022-10-09T13:46:03Z

How about I just rename importer_vectorization.cpp -> importervectorization.cpp, importcall.cpp -> importercalls.cpp and register_arg_convention -> registerargconvention?

Works for me (although I'd prefer capital letters but it's against our guidelines for file names I assume) 👍 maybe someone from @dotnet/jit-contrib have a different opinion?

jakobbotsch · 2022-10-09T14:49:38Z

This change shows some differences in diff algorithms:

❯ git diff 79a78728bd40ae917^..79a78728bd40ae917 --diff-algorithm=myers --shortstat -- .\src\coreclr\jit\importer.cpp
 1 file changed, 8765 insertions(+), 16568 deletions(-)

❯ git diff 79a78728bd40ae917^..79a78728bd40ae917 --diff-algorithm=histogram --shortstat -- .\src\coreclr\jit\importer.cpp
 1 file changed, 7803 deletions(-)

Unfortunately I'm not sure if Git today supports using a different diff algorithm than Myers for git blame/git log -L.

am11 · 2022-10-09T17:48:39Z

A typical way to retain history is to keep the rename and code change commits separate, and merge the PR without squashing (or split file renames into a separate PR).

jakobbotsch · 2022-10-09T17:55:36Z

A typical way to retain history is to keep the rename and code change commits separate, and merge the PR without squashing (or split file renames into a separate PR).

I am not just renaming a file here, I am splitting importer.cpp into importer.cpp and importercalls.cpp. There are no code changes, the problem is that Git's default diff algorithm does not realize that I am just deleting code in importer.cpp and for some reason thinks I am changing the deleted functions into differently named functions from later in the file.

I tried splitting the delete in two commits where the first commit left all comments as sort of "anchors" and the second commit then deleted all the comments. Indeed this would then have to be rebased in. Unfortunately the same problem just happens in the second commit, with Git's default diff algorithm believing I am changing the comments I am deleting into code from later in the file.

jakobbotsch · 2022-10-09T18:09:48Z

I'm trying now to see if I can do the move in a few chunks instead to convince the diff algorithm so that git blame/git log -L will continue working properly (on at least the remaining importer code, and hopefully on the moved code too with some of the args meant for that).

tannergooding · 2022-10-09T18:58:41Z

@jakobbotsch, feel free to rename (or log an issue tracking a rename for) hwintrinsic.cpp to importhwintrinsic.cpp as well.

We might want to decide how to do cases like hwintrinsiccodegenxarch.cpp in that it could be either codegenxarchhwintrinsic.cpp or codegenhwintrinsicxarch.cpp.

Also just noting, as names get longer like this, I think that the _ might help with readability and discoverability of files. Not a huge preference, just noting that since its all lower codegenxarchhwintrinsic is harder to read than codegen_xarch_hwintrinsic.cpp

jakobbotsch · 2022-10-09T19:00:55Z

hwintrinsic.cpp does seem to have some functions that are not related to importation, e.g. Compiler::vnEncodesResultTypeForHWIntrinsic. I think I'll leave it as is for now, we can do follow-up changes if we want to make things more consistent or change the file name conventions.

jakobbotsch · 2022-10-09T19:14:19Z

Ok, moving the functions in batches seems to have worked out, Git properly tracks the deletions in the individual commits. Now we'll just need to rebase/merge this PR.

AndyAyersMS · 2022-10-09T23:20:37Z

Ok, moving the functions in batches seems to have worked out, Git properly tracks the deletions in the individual commits. Now we'll just need to rebase/merge this PR.

Are you planning on not squashing this?

I have some edits for impCheckCanInline in a local branch, will be interesting to see if they can make it into the new location. When I split up flowgraph.cpp it was one big commit and git could not figure out what went where.

AndyAyersMS · 2022-10-10T01:14:44Z

I suppose I can pull down your changes and rebase what I have on top of that.

jakobbotsch · 2022-10-10T08:40:16Z

Are you planning on not squashing this?

I have some edits for impCheckCanInline in a local branch, will be interesting to see if they can make it into the new location. When I split up flowgraph.cpp it was one big commit and git could not figure out what went where.

Yes, that would be the plan. But I don't think Git can figure the code move out automatically, unfortunately.
But at least Git is not going to think I touched the remaining parts of the importer in this PR.

…m file names Also avoid marking some functions as 'inline' since then they cannot be used from other TUs (in general, we can just leave the DCE up to the compiler).

…tReturn

…rinsic, impSRCSUnsafeIntrinsic

…allSite, impCheckForPInvokeCall

…atibleMethodGDV, considerGuardedDevirtualization, addGuardedDevirtualizationCandidate

…getIntrinsic, IsIntrinsicImplementedByUserCall, IsMathIntrinsic, impDevirtualizeCall, impConsiderCallProbe, compClassifyGDVProbeType, impGetSpecialIntrinsicExactReturnType

…Intrinsic, impArrayAccessIntrinsic, impKeepAliveIntrinsic

jakobbotsch · 2022-10-10T18:32:01Z

cc @dotnet/jit-contrib, any final thoughts?

stephentoub · 2022-10-10T21:55:52Z

rebased and merged per @jakobbotsch's request

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

ghost assigned jakobbotsch Oct 9, 2022

jakobbotsch changed the title ~~JIT: Split out call importation into new importcall.cpp~~ JIT: Split out call importation into new importercalls.cpp Oct 9, 2022

jakobbotsch force-pushed the split-out-call-importation branch 2 times, most recently from 0e8a89a to 947ab2c Compare October 9, 2022 18:58

This was referenced Oct 10, 2022

503's dotnet/arcade#10943

Closed

Tracking issue for CI build timeouts #76454

Closed

Azure DevOps Maintenance in dnceng and dnceng-public organizations dotnet/arcade#11188

Closed

jakobbotsch closed this Oct 10, 2022

jakobbotsch reopened this Oct 10, 2022

jakobbotsch force-pushed the split-out-call-importation branch from 947ab2c to 9b677c2 Compare October 10, 2022 12:12

jakobbotsch force-pushed the split-out-call-importation branch from 9b677c2 to f433f35 Compare October 10, 2022 12:47

AndyAyersMS approved these changes Oct 10, 2022

View reviewed changes

jakobbotsch added 10 commits October 10, 2022 20:11

JIT: Add importercalls.cpp for upcoming split, remove underscores fro…

01a7a38

…m file names Also avoid marking some functions as 'inline' since then they cannot be used from other TUs (in general, we can just leave the DCE up to the compiler).

JIT: Move impImportCall, impImportJitTestLabelMark, impFixupCallStruc…

404b3d2

…tReturn

JIT: Move impImportIndirectCall, impPopArgsForUnmanagedCall

15415a9

JIT: Move impInitializeArrayIntrinsic, impCreateSpanIntrinsic, impInt…

a90d2ff

…rinsic, impSRCSUnsafeIntrinsic

JIT: Move impPopCallArgs, impPopReverseCallArgs

bb8aee1

JIT: Move impTransformThis, impCanPInvokeInline, impCanPInvokeInlineC…

9c99434

…allSite, impCheckForPInvokeCall

JIT: Move SpillRetExprHelper, addFatPointerCandidate, pickGDV, isComp…

0af3741

…atibleMethodGDV, considerGuardedDevirtualization, addGuardedDevirtualizationCandidate

JIT: Move impMarkInlineCandidate, impMarkInlineCandidateHelper, IsTar…

96af210

…getIntrinsic, IsIntrinsicImplementedByUserCall, IsMathIntrinsic, impDevirtualizeCall, impConsiderCallProbe, compClassifyGDVProbeType, impGetSpecialIntrinsicExactReturnType

JIT: Move impTailCallRetTypeCompatible, impCheckCanInline

e1f51c2

JIT: Move impMathIntrinsic, lookupNamedIntrinsic, impUnsupportedNamed…

66cca97

…Intrinsic, impArrayAccessIntrinsic, impKeepAliveIntrinsic

jakobbotsch force-pushed the split-out-call-importation branch from f433f35 to 66cca97 Compare October 10, 2022 18:26

jakobbotsch mentioned this pull request Oct 10, 2022

JIT: Use metadata names for SIMD intrinsic method recognition #76786

Merged

stephentoub merged commit d6fdb3a into dotnet:main Oct 10, 2022

jakobbotsch deleted the split-out-call-importation branch October 10, 2022 21:54

ghost locked as resolved and limited conversation to collaborators Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Split out call importation into new importercalls.cpp #76792

JIT: Split out call importation into new importercalls.cpp #76792

jakobbotsch commented Oct 9, 2022 •

edited

Loading

ghost commented Oct 9, 2022

EgorBo commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

EgorBo commented Oct 9, 2022

EgorBo commented Oct 9, 2022

EgorBo commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

EgorBo commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

am11 commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

tannergooding commented Oct 9, 2022 •

edited

Loading

jakobbotsch commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

AndyAyersMS commented Oct 9, 2022

AndyAyersMS commented Oct 10, 2022

jakobbotsch commented Oct 10, 2022

jakobbotsch commented Oct 10, 2022

stephentoub commented Oct 10, 2022

JIT: Split out call importation into new importercalls.cpp #76792

JIT: Split out call importation into new importercalls.cpp #76792

Conversation

jakobbotsch commented Oct 9, 2022 • edited Loading

ghost commented Oct 9, 2022

EgorBo commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

EgorBo commented Oct 9, 2022

EgorBo commented Oct 9, 2022

EgorBo commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

EgorBo commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

am11 commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

tannergooding commented Oct 9, 2022 • edited Loading

jakobbotsch commented Oct 9, 2022

jakobbotsch commented Oct 9, 2022

AndyAyersMS commented Oct 9, 2022

AndyAyersMS commented Oct 10, 2022

jakobbotsch commented Oct 10, 2022

jakobbotsch commented Oct 10, 2022

stephentoub commented Oct 10, 2022

jakobbotsch commented Oct 9, 2022 •

edited

Loading

tannergooding commented Oct 9, 2022 •

edited

Loading