Use jit interface thunkgen tech for more files #45044

davidwrighton · 2020-11-21T01:47:40Z

Reduce the boilerplate logic when modifying the jit ee interface
3 new files in superpmi are now autogenerated as well as 2 files in the jit directory are now autogenerated
Reduce to only having one copy of the jit-ee interface guid

davidwrighton · 2020-11-21T01:48:10Z

@dotnet/jit-contrib

AndyAyersMS

Thanks! I had been thinking we should do something like this for a long time.

AndyAyersMS · 2020-11-21T02:23:17Z

src/coreclr/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h

+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+// DO NOT EDIT THIS FILE! It IS AUTOGENERATED


Can you leave breadcrumbs here and in the other newly auto-generated files to hiht at how they can be regenerated?

Yes, please include pointers to the instructions for re-generation

https://github.com/dotnet/runtime/blob/master/docs/project/updating-jitinterface.md is the current doc if changes are needed there.

AndyAyersMS · 2020-11-21T02:28:09Z

@BruceForstall @sandreenko think you should look this over too.

jkotas · 2020-11-21T02:35:05Z

Unfortunately requires compiling the libjitinterface with the PAL

Why is that? That's very unfortunate.

I think depending on the PAL headers would be ok, but it would be really nice to avoid linking with the PAL. I was hoping that the number of places we link the PAL will decrease over time...

BruceForstall

This should make updating the JIT-EE interface easier, which is much appreciated.

BruceForstall · 2020-11-24T18:26:58Z

src/coreclr/src/tools/Common/JitInterface/ThunkGenerator/Program.cs

+            {
+                tw.Write($"\n{decl.ReturnType.NativeTypeName} WrapICorJitInfo::{ decl.FunctionName}(");
+                bool isFirst = true;
+                foreach (Parameter param in decl.Parameters)


nit? Looks like you could save lots of duplicated code by extracting a function to output function signatures, and function argument call lists

BruceForstall · 2020-11-24T18:28:45Z

src/coreclr/src/tools/Common/JitInterface/ThunkGenerator/Program.cs

-                    WriteManagedThunkInterface(tw, functions);
-                }
-                using (TextWriter tw = new StreamWriter(args[2]))
+


Seems like you should have at least minimal argument validation here, like checking the number of arguments, and a "usage" message that would also serve as a short description of what this tool is doing.

sandreenko

looks good, I like the simplified jit ee interface changes.

sandreenko · 2020-11-24T18:37:57Z

src/coreclr/src/tools/Common/JitInterface/ThunkGenerator/gen.bat

@@ -1,4 +1,4 @@
 pushd %~dp0
-call ..\..\..\..\..\..\..\dotnet.cmd run -- ThunkInput.txt ..\CorInfoBase.cs ..\..\..\aot\jitinterface\jitinterface.h
+call ..\..\..\..\..\..\..\dotnet.cmd run -- ThunkInput.txt ..\CorInfoBase.cs ..\..\..\aot\jitinterface\jitinterface.h ..\..\..\..\jit\ICorJitInfo_API_names.h ..\..\..\..\jit\ICorJitInfo_API_wrapper.hpp ..\..\..\..\ToolBox\superpmi\superpmi-shared\icorjitinfoimpl.h ..\..\..\..\ToolBox\superpmi\superpmi-shim-counter\icorjitinfo.cpp ..\..\..\..\ToolBox\superpmi\superpmi-shim-simple\icorjitinfo.cpp


Nit: name these paths like ..\..\..\..\..\..\..\ etc?

sandreenko · 2020-11-24T18:40:28Z

src/coreclr/src/tools/aot/jitinterface/corinfoexception.h

@@ -9,17 +9,17 @@ typedef char16_t WCHAR;
 typedef wchar_t WCHAR;
 #endif

-class CorInfoException


question: why was it renamed?

CorInfoException is an enum type in corinfo.h

- Removes duplicate copy of jit ee api version guid - Sets up for making jit interface wrapper actually implement the jit interface Update generater to match previous manual modification Use ICorJitInfo interface for jit thunk directly instead of relying on parallel definition to match up - Found a bug in handling of TryResolveToken Generate api names Generate jit api wrapper from thunk generator Generate icorjitinfoimpl.h spmi counter shim icorjitinfo superpmi-shim-simple Update linux script Fix utf8 char out marshalling Builds on Linux but doesn't work Compile with PAL Respond to code review feedback

davidwrighton · 2020-12-01T00:38:58Z

Updated with code review feedback. I've stepped back from requiring the PAL, as that was causing odd problems. I may address that in a future change.

jkotas · 2020-12-01T00:52:20Z

src/coreclr/src/tools/aot/jitinterface/jitinterface.h

-    }
+    virtual unsigned int getMethodAttribs(
+          void* ftn)
+{


Nit: The indentation is off.

The generator is ugly enough as it is. Unless there is pushback, I'd prefer to leave the indentation.

BruceForstall

A few comment-related comments

BruceForstall · 2020-12-01T21:16:03Z

src/coreclr/src/inc/jiteeversionguid.h

@@ -0,0 +1,28 @@
+


Doesn't this file also need a license notice?

BruceForstall · 2020-12-01T21:16:32Z

src/coreclr/src/inc/jiteeversionguid.h

+    0x65a1,
+    0x487a,
+    {0x85, 0x53, 0xc8, 0x45, 0x49, 0x6d, 0xa9, 0x01}
+};


nit: missing trailing end-of-line character?

BruceForstall · 2020-12-01T21:20:25Z

src/coreclr/src/inc/corinfo.h

-    {0x85, 0x53, 0xc8, 0x45, 0x49, 0x6d, 0xa9, 0x01}
-};
-
+#include "jiteeversionguid.h"
 //////////////////////////////////////////////////////////////////////////////////////////////////////////


Seems like the "END JITEEVersionIdentifier" comment needs to be in the jiteeversionguid.h file.

We probably should have big, unmiss-able, comments at the top of all the JIT-EE interface files pointing to the JIT-EE version, but that's somewhat unrelated; this system only works when code reviewers know to ask for an interface GUID change.

Maybe the jiteeversionguid.h file should also point to the updating-jitinterface.md documentation.

With dotnet#45044 it moved from corinfo.h to jiteeversionguid.h

…5576) With #45044 it moved from corinfo.h to jiteeversionguid.h

With #45044 it moved from corinfo.h to jiteeversionguid.h

Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 21, 2020

davidwrighton requested a review from AndyAyersMS November 21, 2020 01:48

AndyAyersMS reviewed Nov 21, 2020

View reviewed changes

BruceForstall mentioned this pull request Nov 24, 2020

Initial version of class profiling for PGO #45133

Merged

BruceForstall reviewed Nov 24, 2020

View reviewed changes

sandreenko reviewed Nov 24, 2020

View reviewed changes

davidwrighton added 2 commits November 30, 2020 16:24

Add some primitive argument checking

6d957aa

davidwrighton force-pushed the better_thunk_gen branch from c0819a1 to 6d957aa Compare December 1, 2020 00:37

jkotas reviewed Dec 1, 2020

View reviewed changes

BruceForstall approved these changes Dec 1, 2020

View reviewed changes

Update comments and license header

fc6b64d

jkotas approved these changes Dec 2, 2020

View reviewed changes

AndyAyersMS approved these changes Dec 2, 2020

View reviewed changes

davidwrighton merged commit e9a47b7 into dotnet:master Dec 2, 2020

AndyAyersMS mentioned this pull request Dec 2, 2020

[WIP] conditionally assign (exact)contextHandle #45321

Closed

BruceForstall added a commit to BruceForstall/runtime that referenced this pull request Dec 4, 2020

Fix location of JIT-EE version GUID

ca83770

With dotnet#45044 it moved from corinfo.h to jiteeversionguid.h

BruceForstall mentioned this pull request Dec 4, 2020

Fix location of JIT-EE version GUID #45574

Merged

BruceForstall added a commit to BruceForstall/runtime that referenced this pull request Dec 4, 2020

SuperPMI collection pipeline: fix location of JIT-EE version GUID

8c3e3ef

With dotnet#45044 it moved from corinfo.h to jiteeversionguid.h

BruceForstall mentioned this pull request Dec 4, 2020

SuperPMI collection pipeline: fix location of JIT-EE version GUID #45576

Merged

BruceForstall added a commit that referenced this pull request Dec 4, 2020

SuperPMI collection pipeline: fix location of JIT-EE version GUID (#4…

a259ec2

…5576) With #45044 it moved from corinfo.h to jiteeversionguid.h

jkotas pushed a commit that referenced this pull request Dec 4, 2020

Fix location of JIT-EE version GUID (#45574)

3cc30f4

With #45044 it moved from corinfo.h to jiteeversionguid.h

ghost locked as resolved and limited conversation to collaborators Jan 1, 2021

davidwrighton deleted the better_thunk_gen branch April 20, 2021 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use jit interface thunkgen tech for more files #45044

Use jit interface thunkgen tech for more files #45044

davidwrighton commented Nov 21, 2020 •

edited

Loading

davidwrighton commented Nov 21, 2020

AndyAyersMS left a comment

AndyAyersMS Nov 21, 2020

BruceForstall Nov 24, 2020

MichalStrehovsky Nov 24, 2020

AndyAyersMS commented Nov 21, 2020

jkotas commented Nov 21, 2020 •

edited

Loading

BruceForstall left a comment

BruceForstall Nov 24, 2020

BruceForstall Nov 24, 2020

sandreenko left a comment

sandreenko Nov 24, 2020

sandreenko Nov 24, 2020

davidwrighton Nov 24, 2020

davidwrighton commented Dec 1, 2020

jkotas Dec 1, 2020

davidwrighton Dec 1, 2020

BruceForstall left a comment

BruceForstall Dec 1, 2020

BruceForstall Dec 1, 2020

BruceForstall Dec 1, 2020

Use jit interface thunkgen tech for more files #45044

Use jit interface thunkgen tech for more files #45044

Conversation

davidwrighton commented Nov 21, 2020 • edited Loading

davidwrighton commented Nov 21, 2020

AndyAyersMS 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

AndyAyersMS commented Nov 21, 2020

jkotas commented Nov 21, 2020 • edited Loading

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

sandreenko 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

davidwrighton commented Dec 1, 2020

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

davidwrighton commented Nov 21, 2020 •

edited

Loading

jkotas commented Nov 21, 2020 •

edited

Loading