Skip to content

Move GenTreeIntCon::gtCompileTimeHandle field to be a global map #115542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 26, 2025
Merged

Move GenTreeIntCon::gtCompileTimeHandle field to be a global map #115542

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5e6e387
remove unused JIT-EE APIs
EgorBo May 14, 2025
991347b
clean up
EgorBo May 15, 2025
a8a58bc
Fix the issue
EgorBo May 15, 2025
ab5c1c0
Update jiteeversionguid.h
EgorBo May 15, 2025
8ad1bec
Update jiteeversionguid.h
EgorBo May 15, 2025
f886f7e
Address feedback
EgorBo May 23, 2025
5cdcb09
Merge branch 'main' into remove-compile-time-handle-fld
EgorBo May 24, 2025
8c8f8d9
Merge branch 'main' into remove-compile-time-handle-fld
EgorBo May 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions src/coreclr/inc/corinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1204,7 +1204,7 @@ enum CorInfoAccessAllowedHelperArgType
CORINFO_HELPER_ARG_TYPE_Field = 1,
CORINFO_HELPER_ARG_TYPE_Method = 2,
CORINFO_HELPER_ARG_TYPE_Class = 3,
CORINFO_HELPER_ARG_TYPE_Module = 4,
// CORINFO_HELPER_ARG_TYPE_Module = 4,
CORINFO_HELPER_ARG_TYPE_Const = 5,
};
struct CORINFO_HELPER_ARG
Expand Down Expand Up @@ -3136,11 +3136,6 @@ class ICorDynamicInfo : public ICorStaticInfo
CORINFO_MODULE_HANDLE handle
) = 0;

virtual CORINFO_MODULE_HANDLE embedModuleHandle(
CORINFO_MODULE_HANDLE handle,
void **ppIndirection = NULL
) = 0;

virtual CORINFO_CLASS_HANDLE embedClassHandle(
CORINFO_CLASS_HANDLE handle,
void **ppIndirection = NULL
Expand All @@ -3151,11 +3146,6 @@ class ICorDynamicInfo : public ICorStaticInfo
void **ppIndirection = NULL
) = 0;

virtual CORINFO_FIELD_HANDLE embedFieldHandle(
CORINFO_FIELD_HANDLE handle,
void **ppIndirection = NULL
) = 0;

// Given a module scope (module), a method handle (context) and
// a metadata token (metaTOK), fetch the handle
// (type, field or method) associated with the token.
Expand Down
8 changes: 0 additions & 8 deletions src/coreclr/inc/icorjitinfoimpl_generated.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,10 +556,6 @@ void* getMethodSync(
CorInfoHelpFunc getLazyStringLiteralHelper(
CORINFO_MODULE_HANDLE handle) override;

CORINFO_MODULE_HANDLE embedModuleHandle(
CORINFO_MODULE_HANDLE handle,
void** ppIndirection) override;

CORINFO_CLASS_HANDLE embedClassHandle(
CORINFO_CLASS_HANDLE handle,
void** ppIndirection) override;
Expand All @@ -568,10 +564,6 @@ CORINFO_METHOD_HANDLE embedMethodHandle(
CORINFO_METHOD_HANDLE handle,
void** ppIndirection) override;

CORINFO_FIELD_HANDLE embedFieldHandle(
CORINFO_FIELD_HANDLE handle,
void** ppIndirection) override;

void embedGenericHandle(
CORINFO_RESOLVED_TOKEN* pResolvedToken,
bool fEmbedParent,
Expand Down
10 changes: 5 additions & 5 deletions src/coreclr/inc/jiteeversionguid.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@

#include <minipal/guid.h>

constexpr GUID JITEEVersionIdentifier = { /* 26d0dde8-bc9d-4543-9b9a-57ad8b1acdc0 */
0x26d0dde8,
0xbc9d,
0x4543,
{0x9b, 0x9a, 0x57, 0xad, 0x8b, 0x1a, 0xcd, 0xc0}
constexpr GUID JITEEVersionIdentifier = { /* bcfedb4f-ed47-4df3-8156-7ad8fc8521f1 */
0xbcfedb4f,
0xed47,
0x4df3,
{0x81, 0x56, 0x7a, 0xd8, 0xfc, 0x85, 0x21, 0xf1}
};

#endif // JIT_EE_VERSIONING_GUID_H
2 changes: 0 additions & 2 deletions src/coreclr/jit/ICorJitInfo_names_generated.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,8 @@ DEF_CLR_API(getFunctionEntryPoint)
DEF_CLR_API(getFunctionFixedEntryPoint)
DEF_CLR_API(getMethodSync)
DEF_CLR_API(getLazyStringLiteralHelper)
DEF_CLR_API(embedModuleHandle)
DEF_CLR_API(embedClassHandle)
DEF_CLR_API(embedMethodHandle)
DEF_CLR_API(embedFieldHandle)
DEF_CLR_API(embedGenericHandle)
DEF_CLR_API(getLocationOfThisType)
DEF_CLR_API(getAddressOfPInvokeTarget)
Expand Down
20 changes: 0 additions & 20 deletions src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1318,16 +1318,6 @@ CorInfoHelpFunc WrapICorJitInfo::getLazyStringLiteralHelper(
return temp;
}

CORINFO_MODULE_HANDLE WrapICorJitInfo::embedModuleHandle(
CORINFO_MODULE_HANDLE handle,
void** ppIndirection)
{
API_ENTER(embedModuleHandle);
CORINFO_MODULE_HANDLE temp = wrapHnd->embedModuleHandle(handle, ppIndirection);
API_LEAVE(embedModuleHandle);
return temp;
}

CORINFO_CLASS_HANDLE WrapICorJitInfo::embedClassHandle(
CORINFO_CLASS_HANDLE handle,
void** ppIndirection)
Expand All @@ -1348,16 +1338,6 @@ CORINFO_METHOD_HANDLE WrapICorJitInfo::embedMethodHandle(
return temp;
}

CORINFO_FIELD_HANDLE WrapICorJitInfo::embedFieldHandle(
CORINFO_FIELD_HANDLE handle,
void** ppIndirection)
{
API_ENTER(embedFieldHandle);
CORINFO_FIELD_HANDLE temp = wrapHnd->embedFieldHandle(handle, ppIndirection);
API_LEAVE(embedFieldHandle);
return temp;
}

void WrapICorJitInfo::embedGenericHandle(
CORINFO_RESOLVED_TOKEN* pResolvedToken,
bool fEmbedParent,
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3003,6 +3003,7 @@ class Compiler
GenTree* gtNewIconEmbHndNode(void* value, void* pValue, GenTreeFlags flags, void* compileTimeHandle);

GenTree* gtNewIconEmbScpHndNode(CORINFO_MODULE_HANDLE scpHnd);
GenTree* gtNewIconEmbObjHndNode(CORINFO_OBJECT_HANDLE objHnd);
GenTree* gtNewIconEmbClsHndNode(CORINFO_CLASS_HANDLE clsHnd);
GenTree* gtNewIconEmbMethHndNode(CORINFO_METHOD_HANDLE methHnd);
GenTree* gtNewIconEmbFldHndNode(CORINFO_FIELD_HANDLE fldHnd);
Expand Down
23 changes: 9 additions & 14 deletions src/coreclr/jit/compiler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1489,13 +1489,7 @@ inline GenTreeIntCon* Compiler::gtNewIconHandleNode(size_t value, GenTreeFlags f

inline GenTree* Compiler::gtNewIconEmbScpHndNode(CORINFO_MODULE_HANDLE scpHnd)
{
void *embedScpHnd, *pEmbedScpHnd;

embedScpHnd = (void*)info.compCompHnd->embedModuleHandle(scpHnd, &pEmbedScpHnd);

assert((!embedScpHnd) != (!pEmbedScpHnd));

return gtNewIconEmbHndNode(embedScpHnd, pEmbedScpHnd, GTF_ICON_SCOPE_HDL, scpHnd);
return gtNewIconEmbHndNode((void*)scpHnd, nullptr, GTF_ICON_SCOPE_HDL, scpHnd);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this introducing an abstraction violation? CORINFO_XXX_HANDLEs are not meant to be embedded in the code. We have an intentional distinction between the CORINFO_XXX_HANDLE handles used to represent type system entities on JIT/EE interface and how these values get embedded into the code.

It happens to work since these module handles are only used with JIT currently, but this change does not look like an improvement to me.

Copy link
Member Author

@EgorBo EgorBo May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkotas I just assumed that almost 300 LOC is too much for an API that is just no-op (and CoreCLR only), I can revert this part.

Looks like the only case where we need to embed a field handle is CoreCLR jitting a method that accesses a field it has no access for, so JIT emits an exception telling about the field access violation. Isn't part of the IL verification that we got rid some time ago?

As for the modules, I presume they're only used to embed strings as helper calls, so JIT only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

300 LOC

It is more like 20 lines of actual code. The rest is boiler plate.

I can revert this part.

I think the part that's introducing JIT/EE interface abstraction violations should be reverted.

Looks like the only case where we need to embed a field handle is CoreCLR jitting a method that accesses a field it has no access for, so JIT emits an exception telling about the field access violation. Isn't part of the IL verification that we got rid some time ago?

We have not removed visibility checks to enforce good hygiene.

CORINFO_HELPER_DESC/impInsertHelperCall is meant to be a general-purpose facility to replace a method call or field access with a call to a helper that throws an exception. Doing it this way allows debugger to stop at the line that failed the visibility check.

If we were to fail the JITing of the whole method instead, the user would just see that there is a visibility problem somewhere in method, without seeing the precise location.

I presume they're only used to embed strings as helper calls, so JIT only.

There is a TODO to implement this optimization for R2R

runtime/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

Lines 3591 to 3592 in 8df4955

// TODO: Lazy string literal helper
return CorInfoHelpFunc.CORINFO_HELP_UNDEF;
.

Where possible, we should be building JIT/EE interactions such that they are JIT/AOT agnostic even if it is not needed at the moment.

}

//-----------------------------------------------------------------------------
Expand All @@ -1513,6 +1507,13 @@ inline GenTree* Compiler::gtNewIconEmbClsHndNode(CORINFO_CLASS_HANDLE clsHnd)

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

inline GenTree* Compiler::gtNewIconEmbObjHndNode(CORINFO_OBJECT_HANDLE objHnd)
{
return gtNewIconEmbHndNode((void*)objHnd, nullptr, GTF_ICON_OBJ_HDL, nullptr);
}

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

inline GenTree* Compiler::gtNewIconEmbMethHndNode(CORINFO_METHOD_HANDLE methHnd)
{
void *embedMethHnd, *pEmbedMethHnd;
Expand All @@ -1528,13 +1529,7 @@ inline GenTree* Compiler::gtNewIconEmbMethHndNode(CORINFO_METHOD_HANDLE methHnd)

inline GenTree* Compiler::gtNewIconEmbFldHndNode(CORINFO_FIELD_HANDLE fldHnd)
{
void *embedFldHnd, *pEmbedFldHnd;

embedFldHnd = (void*)info.compCompHnd->embedFieldHandle(fldHnd, &pEmbedFldHnd);

assert((!embedFldHnd) != (!pEmbedFldHnd));

return gtNewIconEmbHndNode(embedFldHnd, pEmbedFldHnd, GTF_ICON_FIELD_HDL, fldHnd);
return gtNewIconEmbHndNode((void*)fldHnd, nullptr, GTF_ICON_FIELD_HDL, fldHnd);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

}

/*****************************************************************************/
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/flowgraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1284,9 +1284,9 @@ GenTree* Compiler::fgGetCritSectOfStaticMethod()
if (!kind.needsRuntimeLookup)
{
CORINFO_OBJECT_HANDLE ptr = info.compCompHnd->getRuntimeTypePointer(info.compClassHnd);
if (ptr != NULL)
if (ptr != NO_OBJECT_HANDLE)
{
tree = gtNewIconEmbHndNode((void*)ptr, nullptr, GTF_ICON_OBJ_HDL, nullptr);
tree = gtNewIconEmbObjHndNode(ptr);
}
else
{
Expand Down
12 changes: 2 additions & 10 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7772,8 +7772,7 @@ GenTree* Compiler::gtNewStringLiteralNode(InfoAccessType iat, void* pValue)
switch (iat)
{
case IAT_VALUE:
tree = gtNewIconEmbHndNode(pValue, nullptr, GTF_ICON_OBJ_HDL, nullptr);
INDEBUG(tree->AsIntCon()->gtTargetHandle = (size_t)pValue);
tree = gtNewIconEmbObjHndNode((CORINFO_OBJECT_HANDLE)pValue);
break;

case IAT_PVALUE: // The value needs to be accessed via an indirection
Expand Down Expand Up @@ -8137,14 +8136,7 @@ GenTree* Compiler::gtNewGenericCon(var_types type, uint8_t* cnsVal)
case TYP_REF:
{
READ_VALUE(ssize_t);
if (val == 0)
{
return gtNewNull();
}
else
{
return gtNewIconEmbHndNode((void*)val, nullptr, GTF_ICON_OBJ_HDL, nullptr);
}
return val == 0 ? gtNewNull() : gtNewIconEmbObjHndNode((CORINFO_OBJECT_HANDLE)val);
}
#ifdef FEATURE_SIMD
case TYP_SIMD8:
Expand Down
3 changes: 0 additions & 3 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4350,9 +4350,6 @@ void Compiler::impInsertHelperCall(CORINFO_HELPER_DESC* helperInfo)
info.compCompHnd->classMustBeLoadedBeforeCodeIsRun(helperArg.classHandle);
currentArg = gtNewIconEmbClsHndNode(helperArg.classHandle);
break;
case CORINFO_HELPER_ARG_TYPE_Module:
currentArg = gtNewIconEmbScpHndNode(helperArg.moduleHandle);
break;
case CORINFO_HELPER_ARG_TYPE_Const:
currentArg = gtNewIconNode(helperArg.constant);
break;
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/importercalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8519,8 +8519,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
// Note different embedding would be needed for NAOT/R2R,
// but we have ruled those out above.
//
GenTree* const instParam =
gtNewIconEmbHndNode(instantiatingStub, nullptr, GTF_ICON_METHOD_HDL, instantiatingStub);
GenTree* const instParam = gtNewIconEmbMethHndNode(instantiatingStub);
call->gtArgs.InsertInstParam(this, instParam);
}

Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6484,8 +6484,7 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call)
CORINFO_OBJECT_HANDLE ptr = info.compCompHnd->getRuntimeTypePointer(hClass);
if (ptr != NULL)
{
GenTree* retNode = gtNewIconEmbHndNode((void*)ptr, nullptr, GTF_ICON_OBJ_HDL, nullptr);
return fgMorphTree(retNode);
return fgMorphTree(gtNewIconEmbObjHndNode(ptr));
}
}
}
Expand Down
22 changes: 14 additions & 8 deletions src/coreclr/jit/valuenum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2708,9 +2708,9 @@ ValueNum ValueNumStore::VNForCast(VNFunc func, ValueNum castToVN, ValueNum objVN
bool isExact;
bool isNonNull;
CORINFO_CLASS_HANDLE castFrom = GetObjectType(objVN, &isExact, &isNonNull);
CORINFO_CLASS_HANDLE castTo;
CORINFO_CLASS_HANDLE castTo = NO_CLASS_HANDLE;
if ((castFrom != NO_CLASS_HANDLE) &&
EmbeddedHandleMapLookup(ConstantValue<ssize_t>(castToVN), (ssize_t*)&castTo))
EmbeddedHandleMapLookup(ConstantValue<ssize_t>(castToVN), (ssize_t*)&castTo) && (castTo != NO_CLASS_HANDLE))
{
TypeCompareState castResult = m_pComp->info.compCompHnd->compareTypesForCast(castFrom, castTo);
if (castResult == TypeCompareState::Must)
Expand Down Expand Up @@ -13738,9 +13738,12 @@ bool Compiler::fgValueNumberSpecialIntrinsic(GenTreeCall* call)
break;
}

ValueNum clsVN = typeHandleFuncApp.m_args[0];
ssize_t clsHandle;
if (!vnStore->EmbeddedHandleMapLookup(vnStore->ConstantValue<ssize_t>(clsVN), &clsHandle))
ValueNum clsVN = typeHandleFuncApp.m_args[0];
ssize_t clsHandle = 0;

// NOTE: EmbeddedHandleMapLookup may return 0 for non-0 embedded handle
if (!vnStore->EmbeddedHandleMapLookup(vnStore->ConstantValue<ssize_t>(clsVN), &clsHandle) &&
(clsHandle != 0))
{
break;
}
Expand Down Expand Up @@ -15152,9 +15155,12 @@ CORINFO_CLASS_HANDLE ValueNumStore::GetObjectType(ValueNum vn, bool* pIsExact, b
const VNFunc func = funcApp.m_func;
if ((func == VNF_CastClass) || (func == VNF_IsInstanceOf) || (func == VNF_JitNew))
{
ssize_t clsHandle;
ValueNum clsVN = funcApp.m_args[0];
if (IsVNTypeHandle(clsVN) && EmbeddedHandleMapLookup(ConstantValue<ssize_t>(clsVN), &clsHandle))
ssize_t clsHandle = 0;
ValueNum clsVN = funcApp.m_args[0];

// NOTE: EmbeddedHandleMapLookup may return 0 for non-0 embedded handle
if (IsVNTypeHandle(clsVN) && EmbeddedHandleMapLookup(ConstantValue<ssize_t>(clsVN), &clsHandle) &&
(clsHandle != 0))
{
// JitNew returns an exact and non-null obj, castclass and isinst do not have this guarantee.
*pIsNonNull = func == VNF_JitNew;
Expand Down
6 changes: 0 additions & 6 deletions src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3592,12 +3592,6 @@ private CorInfoHelpFunc getLazyStringLiteralHelper(CORINFO_MODULE_STRUCT_* handl
return CorInfoHelpFunc.CORINFO_HELP_UNDEF;
}

private CORINFO_MODULE_STRUCT_* embedModuleHandle(CORINFO_MODULE_STRUCT_* handle, ref void* ppIndirection)
{ throw new NotImplementedException("embedModuleHandle"); }

private CORINFO_FIELD_STRUCT_* embedFieldHandle(CORINFO_FIELD_STRUCT_* handle, ref void* ppIndirection)
{ throw new NotImplementedException("embedFieldHandle"); }

private static CORINFO_RUNTIME_LOOKUP_KIND GetGenericRuntimeLookupKind(MethodDesc method)
{
if (method.RequiresInstMethodDescArg())
Expand Down
Loading
Loading