new JitEE interface method: expandRawHandleIntrinsic #12071

sandreenko · 2017-06-03T00:46:44Z

It is implemented only in CoreRT and used in importer.

Delete isDelegateCreationAllowed because it is unused and I was waiting for a JitEEinterface change
to delete it.

It became unused after dotnet#10626 (delete GetMethod2).

sandreenko · 2017-06-03T00:48:24Z

src/jit/importer.cpp

@@ -6675,6 +6703,12 @@ var_types Compiler::impImportCall(OPCODE                  opcode,
            call = impIntrinsic(newobjThis, clsHnd, methHnd, sig, pResolvedToken->token, readonlyCall,
                                (canTailCall && (tailCall != 0)), &intrinsicID);

+            if (compInlineResult->IsFailure())


It is because now we call impLookupToTree inside impIntrinsic, that can set the Failure if it is compIsForInlining() and we need a runtime lookup.

sandreenko · 2017-06-03T00:50:27Z

I didn't notice any order in these files (and code convention too), so I just added my method right after the last added resolveVirtualMethod.

sandreenko · 2017-06-03T00:51:48Z

src/jit/importer.cpp

+        {
+            assert(IsTargetAbi(CORINFO_CORERT_ABI)); // Only CoreRT supports it.
+            CORINFO_RESOLVED_TOKEN resolvedToken;
+            resolvedToken.hMethod      = method;


@MichalStrehovsky Is it necessary to set hMethod here? It is usually an output argument for resolvedToken.

You can null it out. We can fetch that from token/tokenScope (and in fact, we need do it from the token anyway if the method is canonical (and it pretty much always will be canonical)).

Cool, done.

sandreenko · 2017-06-03T00:52:37Z

PTAL @dotnet/jit-contrib, @MichalStrehovsky. @jkotas.

jkotas · 2017-06-03T01:14:29Z

src/inc/corinfo.h

@@ -2111,13 +2121,6 @@ class ICorStaticInfo
            BOOL                        *pfIsOpenDelegate /* is the delegate open */
            ) = 0;

-    // Determines whether the delegate creation obeys security transparency rules


While you are on it you can also delete getAddrModuleDomainID ... I have noticed earlier today that it is also unused.

Thanks, done.

jkotas · 2017-06-03T01:19:37Z

LGTM

sandreenko · 2017-06-03T01:23:09Z

I think I do not understand the purpose of jit\ICorJitInfo_API_names.h, @AndyAyersMS do you know that? Why is not resolveVirtualMethod there?

MichalStrehovsky · 2017-06-03T01:00:21Z

src/jit/importer.cpp

+        {
+            assert(IsTargetAbi(CORINFO_CORERT_ABI)); // Only CoreRT supports it.
+            CORINFO_RESOLVED_TOKEN resolvedToken;
+            resolvedToken.hMethod      = method;


You can null it out. We can fetch that from token/tokenScope (and in fact, we need do it from the token anyway if the method is canonical (and it pretty much always will be canonical)).

MichalStrehovsky · 2017-06-03T01:21:11Z

src/jit/importer.cpp

+            if (eeTypePtrOfNode == nullptr)
+                return nullptr;
+
+            unsigned eeSlot = lvaGrabTemp(true DEBUGARG("eeTypePtrOf"));


Nit: could you remove the references to EETypePtrOf?

While the original motivation for this is to allow having a EETypePtr EETypePtrOf<T>() method that intrinsically expands to "raw value of the TypeHandle of T", I'm already thinking of using this for IntPtr DefaultConstructorOf<T>() that expands to "pointer to the default constructor of type T" (to be used in the implementation of Activator.CreateInstance<T>). The API surface is general purpose to allow adding other interesting cases without requiring RyuJIT to learn about them.

Ok, is rawPointer an appropriate name?

MichalStrehovsky · 2017-06-03T01:24:50Z

src/jit/importer.cpp

+            impAssignTempGen(eeSlot, eeTypePtrOfNode, clsHnd, (unsigned)CHECK_SPILL_NONE);
+            GenTreePtr lclVar     = gtNewLclvNode(eeSlot, TYP_I_IMPL);
+            GenTreePtr lclVarAddr = gtNewOperNode(GT_ADDR, TYP_I_IMPL, lclVar);
+            var_types  resultType = JITtype2varType(sig->retType);


Would it be possible to add some kind of assert that the return type of the method is interchangeable with TYP_I_IMPL? (either an IntPtr, or a pointer-sized struct that is ideally not a GC pointer).

I am not sure about a GC pointer check.
I added noway_assert that the return tree has the same size as TYP_I_IMPL.

and the additional assert. It should be noway_assert, because CoreRT doesn't use this intrinsic in the debug mode.

sandreenko · 2017-06-05T16:50:58Z

@dotnet-bot test Windows_NT arm64 Checked pri1r2r

fot the future usages.

sandreenko · 2017-06-05T17:57:07Z

@dotnet-bot test Windows_NT arm64 Checked pri1r2r

sandreenko · 2017-06-05T17:59:08Z

@dotnet-bot test Windows_NT Checked pri1r2r

BruceForstall · 2017-06-05T20:45:37Z

Please port these changes to the desktop and build at least x86/x64/arm64 checked/release there before merging this (to ensure there are no desktop-specific build problems that will ensue when this is mirrored).

BruceForstall · 2017-06-05T20:48:30Z

src/jit/importer.cpp

+        case CORINFO_INTRINSIC_GetRawHandle:
+        {
+            assert(IsTargetAbi(CORINFO_CORERT_ABI)); // Only CoreRT supports it.
+            CORINFO_RESOLVED_TOKEN resolvedToken;


Seems like this should be a noway_assert

BruceForstall · 2017-06-05T20:50:36Z

src/jit/ICorJitInfo_API_wrapper.hpp

@@ -189,17 +189,6 @@ BOOL WrapICorJitInfo::isCompatibleDelegate(
    return temp;
 }

-BOOL WrapICorJitInfo::isDelegateCreationAllowed(
-            CORINFO_CLASS_HANDLE        delegateHnd,


Shouldn't you add a new case for expandRawHandleIntrinsic() in this file?

BruceForstall · 2017-06-05T20:50:53Z

src/jit/ICorJitInfo_API_names.h

@@ -20,7 +20,6 @@ DEF_CLR_API(getUnmanagedCallConv)
 DEF_CLR_API(pInvokeMarshalingRequired)
 DEF_CLR_API(satisfiesMethodConstraints)
 DEF_CLR_API(isCompatibleDelegate)
-DEF_CLR_API(isDelegateCreationAllowed)
 DEF_CLR_API(isInstantiationOfVerifiedGeneric)


Shouldn't you add a new case for expandRawHandleIntrinsic() in this file?

I am not sure, I asked @AndyAyersMS in the message above why there is no resolveVirtualMethod there. Looks like we do not test code under MEASURE_CLRAPI_CALLS 1.

This was created by Peter Kukol to measure JIT-EE interface overhead. It's probably reasonable to keep it working as close as possible, even if we don't actively build with it regularly.

Ok, I fixed that for both methods. I checked that it is possible to build VM with this flag on.

BruceForstall · 2017-06-05T20:53:01Z

When this is merged, please be prepared to immediately do a desktop SuperPMI recollection.

add resolveVirtualMethod and expandRawHandleIntrinsic there.

sandreenko · 2017-06-05T23:46:30Z

Please port these changes to the desktop and build at least x86/x64/arm64 checked/release there before merging this (to ensure there are no desktop-specific build problems that will ensue when this is mirrored).

Done for checked, it is in progress for release.

CoreRT side of dotnet/coreclr#12071. I also took this as an opportunity to clean up how we use `runtimeLookupArgs` - we now consistently fill it out instead of trying to figure out what to put there based on the token and ready to run helper ID.

Sergey Andreenko added 4 commits June 1, 2017 12:07

Delete isDelegateCreationAllowed from the jitEEinterface.

4ac0b4f

It became unused after dotnet#10626 (delete GetMethod2).

Create a new JitEE method expandRawHandleIntrinsic.

4c1bc22

support CORINFO_INTRINSIC_GetRawHandle

cd52f3f

change JITEEVersionIdentifier

184e651

dnfclas added the cla-already-signed label Jun 3, 2017

sandreenko added area-CodeGen area-VM labels Jun 3, 2017

sandreenko commented Jun 3, 2017

View reviewed changes

Fix ubuntu formatting

3222668

jkotas reviewed Jun 3, 2017

View reviewed changes

delete getAddrModuleDomainID

14676f3

MichalStrehovsky approved these changes Jun 3, 2017

View reviewed changes

Sergey Andreenko added 2 commits June 2, 2017 18:49

do not init an out arg.

4d84341

and the additional assert. It should be noway_assert, because CoreRT doesn't use this intrinsic in the debug mode.

fix the nullptr error

605fbcd

sandreenko requested a review from BruceForstall June 5, 2017 17:35

MichalStrehovsky mentioned this pull request Jun 5, 2017

Remove relocations from SECTION_MethodDesc for ngened images for fields accessed from jit code #11963

Merged

rename eeTypePtrOfNode to rawHandle

f4c24b0

fot the future usages.

BruceForstall reviewed Jun 5, 2017

View reviewed changes

Sergey Andreenko added 2 commits June 5, 2017 16:43

fix MEASURE_CLRAPI_CALLS

713026e

add resolveVirtualMethod and expandRawHandleIntrinsic there.

use noway_assert for release

f98ab6a

BruceForstall approved these changes Jun 5, 2017

View reviewed changes

sandreenko merged commit b23a11e into dotnet:master Jun 6, 2017

MichalStrehovsky mentioned this pull request Jun 7, 2017

Implement EETypePtrOf intrinsic dotnet/corert#3814

Merged

karelz modified the milestone: 2.1.0 Aug 28, 2017

sandreenko deleted the jitEEInterfaceChanges branch November 2, 2018 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new JitEE interface method: expandRawHandleIntrinsic #12071

new JitEE interface method: expandRawHandleIntrinsic #12071

sandreenko commented Jun 3, 2017

sandreenko Jun 3, 2017 •

edited

Loading

sandreenko commented Jun 3, 2017

sandreenko Jun 3, 2017

MichalStrehovsky Jun 3, 2017

sandreenko Jun 3, 2017

sandreenko commented Jun 3, 2017

jkotas Jun 3, 2017

sandreenko Jun 3, 2017

jkotas commented Jun 3, 2017

sandreenko commented Jun 3, 2017 •

edited

Loading

MichalStrehovsky Jun 3, 2017

MichalStrehovsky Jun 3, 2017

sandreenko Jun 3, 2017

MichalStrehovsky Jun 3, 2017

sandreenko Jun 3, 2017

sandreenko commented Jun 5, 2017

sandreenko commented Jun 5, 2017

sandreenko commented Jun 5, 2017

BruceForstall commented Jun 5, 2017

BruceForstall Jun 5, 2017

sandreenko Jun 5, 2017

BruceForstall Jun 5, 2017

sandreenko Jun 5, 2017

BruceForstall Jun 5, 2017

sandreenko Jun 5, 2017

BruceForstall Jun 5, 2017

sandreenko Jun 5, 2017

BruceForstall commented Jun 5, 2017

sandreenko commented Jun 5, 2017

new JitEE interface method: expandRawHandleIntrinsic #12071

new JitEE interface method: expandRawHandleIntrinsic #12071

Conversation

sandreenko commented Jun 3, 2017

sandreenko Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

sandreenko commented Jun 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko commented Jun 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Jun 3, 2017

sandreenko commented Jun 3, 2017 • edited Loading

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

sandreenko commented Jun 5, 2017

sandreenko commented Jun 5, 2017

sandreenko commented Jun 5, 2017

BruceForstall commented Jun 5, 2017

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

BruceForstall commented Jun 5, 2017

sandreenko commented Jun 5, 2017

sandreenko Jun 3, 2017 •

edited

Loading

sandreenko commented Jun 3, 2017 •

edited

Loading