Skip to content
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.

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

JIT: import static readonly fields holding frozen objects as const handles #76112

Merged
merged 70 commits into from
Oct 10, 2022
Merged
Show file tree
Hide file tree
Changes from 51 commits
Commits
Show all changes
70 commits
Select commit Hold shift + click to select a range
0fdefe2
handle static readonly for frozen objects in jit
EgorBo Sep 24, 2022
07fdafc
Address Jan's feedback
EgorBo Sep 24, 2022
ad69145
Address feedback
EgorBo Sep 24, 2022
f10b886
Mitigate regressions via getObjectType
EgorBo Sep 24, 2022
9bf46c6
Merge branch 'main' of github.com:dotnet/runtime into frozen-static-r…
EgorBo Sep 24, 2022
97c3cdf
use memcpy
EgorBo Sep 24, 2022
fd7e742
Address feedback
EgorBo Sep 24, 2022
75c40ae
make jit-format happy
EgorBo Sep 24, 2022
283b833
Fix FitsI32 assert
EgorBo Sep 24, 2022
00e3e69
clean up in methodcontext
EgorBo Sep 24, 2022
f7902d2
Update importer.cpp
EgorBo Sep 24, 2022
bf7947f
Implement getObjectType for ILC
EgorBo Sep 24, 2022
3f756d9
Merge branch 'frozen-static-readonly' of github.com:EgorBo/runtime-1 …
EgorBo Sep 24, 2022
5374bea
fix clang/gcc
EgorBo Sep 24, 2022
548dc9c
Update src/coreclr/vm/jitinterface.cpp
EgorBo Sep 24, 2022
1234e97
Address feedback
EgorBo Sep 24, 2022
1c584a1
Initial impl for NativeAOT
EgorBo Sep 25, 2022
1fc50c7
Use PreinitializationInfo
EgorBo Sep 25, 2022
600209f
Handle strings
EgorBo Sep 25, 2022
4cad20a
Update src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs
EgorBo Sep 25, 2022
28f2749
Apply suggestions from code review
EgorBo Sep 25, 2022
594a6c2
Merge branch 'frozen-static-readonly' of github.com:EgorBo/runtime-1 …
EgorBo Sep 25, 2022
a07232d
clean up
EgorBo Sep 25, 2022
0c6fe27
Add a comment
EgorBo Sep 25, 2022
271f442
Fix build
EgorBo Sep 25, 2022
76f6cdc
Fix build
EgorBo Sep 25, 2022
133c268
Address Michal's feedback
EgorBo Sep 26, 2022
8880954
Address Michal's feedback
EgorBo Sep 26, 2022
63cd7cc
Simplify PM switch
EgorBo Sep 26, 2022
2656719
Implement getObjectType for FrozenObjectNode
EgorBo Sep 26, 2022
1006572
Address feedback
EgorBo Sep 26, 2022
543a5f5
Merge branch 'main' of github.com:dotnet/runtime into frozen-static-r…
EgorBo Sep 26, 2022
0d27470
Add isObjectImmutable
EgorBo Sep 26, 2022
48343ee
Update gcinfo.cpp
EgorBo Sep 26, 2022
7264f67
fix getRuntimeTypePointer on NativeAOT
EgorBo Sep 26, 2022
9aa1b6c
Merge branch 'frozen-static-readonly' of github.com:EgorBo/runtime-1 …
EgorBo Sep 26, 2022
8d4048a
fix getRuntimeTypePointer on NativeAOT
EgorBo Sep 26, 2022
edb4cd3
Merge branch 'main' of github.com:dotnet/runtime into frozen-static-r…
EgorBo Sep 26, 2022
0e75429
Merge branch 'main' of github.com:dotnet/runtime into frozen-static-r…
EgorBo Oct 6, 2022
44a8fc6
Fix compilation
EgorBo Oct 6, 2022
256bbd4
Apply suggestions from code review
EgorBo Oct 6, 2022
fefbbe1
ILC: use correct TargetPtrSize
EgorBo Oct 6, 2022
3d06a33
Address feedback
EgorBo Oct 6, 2022
afefe44
Add support for delegates, empty arrays and objects without fields
EgorBo Oct 6, 2022
eece561
Add some comments
EgorBo Oct 6, 2022
e2ae117
Update src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TypePreinit.cs
EgorBo Oct 6, 2022
34b39cb
Apply suggestions from code review
EgorBo Oct 6, 2022
22baa43
Ah, actually we need that condition, it can be a struct
EgorBo Oct 6, 2022
4752354
Address Jakob's feedback
EgorBo Oct 7, 2022
49ab023
fix compilation
EgorBo Oct 7, 2022
08d59f5
Use AssertMapExistsNoMessage
EgorBo Oct 7, 2022
45cd02b
Address feedback
EgorBo Oct 7, 2022
b578780
Update src/coreclr/vm/jitinterface.cpp
EgorBo Oct 7, 2022
84de4d2
Address feedback
EgorBo Oct 7, 2022
05e2639
fix compilation
EgorBo Oct 7, 2022
771fd6a
Update src/coreclr/vm/jitinterface.cpp
EgorBo Oct 7, 2022
1acf664
Update src/coreclr/vm/jitinterface.cpp
EgorBo Oct 7, 2022
806e985
Address feedback
EgorBo Oct 7, 2022
2a0fc88
add assert
EgorBo Oct 7, 2022
fe169c6
limit to primitives and objects on llc side
EgorBo Oct 7, 2022
30fbd21
fix check
EgorBo Oct 7, 2022
7c2d8dc
Apply suggestions from code review
EgorBo Oct 8, 2022
be7c16f
Fix indention since "if" was removed
EgorBo Oct 8, 2022
5fbe665
Apply suggestions from code review
EgorBo Oct 8, 2022
56908b0
Address feedback, align NativeAOT logic with CoreCLR
EgorBo Oct 8, 2022
cbe5cd0
Update src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoIm…
EgorBo Oct 8, 2022
76a72d8
Update src/coreclr/vm/jitinterface.cpp
jkotas Oct 8, 2022
aa5be51
Keep IsThreadStatic on vm side
EgorBo Oct 8, 2022
0f8b7cd
Merge branch 'main' of github.com:dotnet/runtime into frozen-static-r…
EgorBo Oct 9, 2022
e4ce64d
Merge branch 'main' of github.com:dotnet/runtime into frozen-static-r…
EgorBo Oct 10, 2022
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
47 changes: 47 additions & 0 deletions src/coreclr/inc/corinfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -2503,6 +2503,32 @@ class ICorStaticInfo
CORINFO_CLASS_HANDLE cls
) = 0;

//------------------------------------------------------------------------------
// isObjectImmutable: checks whether given object is known to be immutable or not
//
// Arguments:
// objPtr - Direct object handle
//
// Return Value:
// Returns true if object is known to be immutable
//
virtual bool isObjectImmutable(
void* objPtr
) = 0;

//------------------------------------------------------------------------------
// getObjectType: obtains type handle for given object
//
// Arguments:
// objPtr - Direct object handle
//
// Return Value:
// Returns CORINFO_CLASS_HANDLE handle that represents given object's type
//
virtual CORINFO_CLASS_HANDLE getObjectType(
void* objPtr
) = 0;

virtual bool getReadyToRunHelper(
CORINFO_RESOLVED_TOKEN * pResolvedToken,
CORINFO_LOOKUP_KIND * pGenericLookupKind,
Expand Down Expand Up @@ -3167,6 +3193,27 @@ class ICorDynamicInfo : public ICorStaticInfo
void **ppIndirection = NULL
) = 0;

//------------------------------------------------------------------------------
// getReadonlyStaticFieldValue: returns true and the actual field's value if the given
// field represents a statically initialized readonly field of any type, it might be:
// * integer/floating point primitive
// * null
// * frozen object reference (string, array or object)
//
// Arguments:
// field - field handle
// buffer - buffer field's value will be stored to
// bufferSize - size of buffer
//
// Return Value:
// Returns true if field's constant value was available and successfully copied to buffer
//
virtual bool getReadonlyStaticFieldValue(
CORINFO_FIELD_HANDLE field,
uint8_t *buffer,
int bufferSize
) = 0;

// If pIsSpeculative is NULL, return the class handle for the value of ref-class typed
// static readonly fields, if there is a unique location for the static and the class
// is already initialized.
Expand Down
11 changes: 11 additions & 0 deletions src/coreclr/inc/icorjitinfoimpl_generated.h
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,12 @@ CorInfoHelpFunc getUnBoxHelper(
void* getRuntimeTypePointer(
CORINFO_CLASS_HANDLE cls) override;

bool isObjectImmutable(
void* objPtr) override;

CORINFO_CLASS_HANDLE getObjectType(
void* objPtr) override;

bool getReadyToRunHelper(
CORINFO_RESOLVED_TOKEN* pResolvedToken,
CORINFO_LOOKUP_KIND* pGenericLookupKind,
Expand Down Expand Up @@ -606,6 +612,11 @@ void* getFieldAddress(
CORINFO_FIELD_HANDLE field,
void** ppIndirection) override;

bool getReadonlyStaticFieldValue(
CORINFO_FIELD_HANDLE field,
uint8_t* buffer,
int bufferSize) override;

CORINFO_CLASS_HANDLE getStaticFieldCurrentClass(
CORINFO_FIELD_HANDLE field,
bool* pIsSpeculative) override;
Expand Down
10 changes: 5 additions & 5 deletions src/coreclr/inc/jiteeversionguid.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,11 @@ typedef const GUID *LPCGUID;
#define GUID_DEFINED
#endif // !GUID_DEFINED

constexpr GUID JITEEVersionIdentifier = { /* 3f5e4630-b29a-4aeb-bab7-07bdff43a156 */
0x3f5e4630,
0xb29a,
0x4aeb,
{0xba, 0xb7, 0x7, 0xbd, 0xff, 0x43, 0xa1, 0x56}
constexpr GUID JITEEVersionIdentifier = { /* 982ed1fd-7bf3-425b-8b8a-902873151e79 */
0x982ed1fd,
0x7bf3,
0x425b,
{0x8b, 0x8a, 0x90, 0x28, 0x73, 0x15, 0x1e, 0x79}
};

//////////////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/jit/ICorJitInfo_API_names.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ DEF_CLR_API(getTypeForBox)
DEF_CLR_API(getBoxHelper)
DEF_CLR_API(getUnBoxHelper)
DEF_CLR_API(getRuntimeTypePointer)
DEF_CLR_API(isObjectImmutable)
DEF_CLR_API(getObjectType)
DEF_CLR_API(getReadyToRunHelper)
DEF_CLR_API(getReadyToRunDelegateCtorHelper)
DEF_CLR_API(getHelperName)
Expand Down Expand Up @@ -152,6 +154,7 @@ DEF_CLR_API(canAccessFamily)
DEF_CLR_API(isRIDClassDomainID)
DEF_CLR_API(getClassDomainID)
DEF_CLR_API(getFieldAddress)
DEF_CLR_API(getReadonlyStaticFieldValue)
DEF_CLR_API(getStaticFieldCurrentClass)
DEF_CLR_API(getVarArgsHandle)
DEF_CLR_API(canGetVarArgsHandle)
Expand Down
29 changes: 29 additions & 0 deletions src/coreclr/jit/ICorJitInfo_API_wrapper.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,24 @@ void* WrapICorJitInfo::getRuntimeTypePointer(
return temp;
}

bool WrapICorJitInfo::isObjectImmutable(
void* objPtr)
{
API_ENTER(isObjectImmutable);
bool temp = wrapHnd->isObjectImmutable(objPtr);
API_LEAVE(isObjectImmutable);
return temp;
}

CORINFO_CLASS_HANDLE WrapICorJitInfo::getObjectType(
void* objPtr)
{
API_ENTER(getObjectType);
CORINFO_CLASS_HANDLE temp = wrapHnd->getObjectType(objPtr);
API_LEAVE(getObjectType);
return temp;
}

bool WrapICorJitInfo::getReadyToRunHelper(
CORINFO_RESOLVED_TOKEN* pResolvedToken,
CORINFO_LOOKUP_KIND* pGenericLookupKind,
Expand Down Expand Up @@ -1451,6 +1469,17 @@ void* WrapICorJitInfo::getFieldAddress(
return temp;
}

bool WrapICorJitInfo::getReadonlyStaticFieldValue(
CORINFO_FIELD_HANDLE field,
uint8_t* buffer,
int bufferSize)
{
API_ENTER(getReadonlyStaticFieldValue);
bool temp = wrapHnd->getReadonlyStaticFieldValue(field, buffer, bufferSize);
API_LEAVE(getReadonlyStaticFieldValue);
return temp;
}

CORINFO_CLASS_HANDLE WrapICorJitInfo::getStaticFieldCurrentClass(
CORINFO_FIELD_HANDLE field,
bool* pIsSpeculative)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -3637,7 +3637,7 @@ class Compiler

GenTree* impInitClass(CORINFO_RESOLVED_TOKEN* pResolvedToken);

GenTree* impImportStaticReadOnlyField(void* fldAddr, var_types lclTyp);
GenTree* impImportStaticReadOnlyField(uint8_t* buffer, int bufferSize, var_types valueType);

GenTree* impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedToken,
CORINFO_ACCESS_FLAGS access,
Expand Down
15 changes: 15 additions & 0 deletions src/coreclr/jit/gcinfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,23 @@ GCInfo::WriteBarrierForm GCInfo::gcIsWriteBarrierCandidate(GenTreeStoreInd* stor
return WBF_NoBarrier;
}

// Write-barriers are no-op for frozen objects (as values)
if (store->Data()->IsIconHandle(GTF_ICON_OBJ_HDL))
{
#ifndef TARGET_XARCH
const ssize_t handle = store->Data()->AsIntCon()->IconValue();
if (!compiler->info.compCompHnd->isObjectImmutable(reinterpret_cast<void*>(handle)))
{
// On platforms with weaker memory model we need to make sure we use a store with the release semantic
// when we publish a potentially mutable object
// See relevant discussions https://github.com/dotnet/runtime/pull/76135#issuecomment-1257258310 and
// https://github.com/dotnet/runtime/pull/76112#discussion_r980639782

// This can be relaxed to "just make sure to use stlr/memory barrier" if needed
store->gtFlags |= GTF_IND_VOLATILE;
Copy link
Member

Choose a reason for hiding this comment

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

This will work, but later it may be relaxed. This is still an ordinary store, unless it was volatile in IL. We just need to emit it as release, but can do all kind of optimizations. It might need a new flag.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: If we need to emit it as release, it is not really an ordinary store.

Copy link
Member

Choose a reason for hiding this comment

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

What I meant - if it was a regular store in IL, we can still move it around or coalesce with other writes to same location - for optimization purposes it is still a regular store. I think GTF_IND_VOLATILE would suppress all that, but it could be ok for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, thanks, by the time we reach this phase we already have done all the transformations/optimizations so GTF_IND_VOLATILE should not really cause codegen diffs apart from the str -> stlr on arm

Copy link
Member Author

@EgorBo EgorBo Oct 6, 2022

Choose a reason for hiding this comment

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

image

Seems to work, here jit decided to use str+dmb instead of stlr because of addressing mode
PS: printObjectDescription() can be improved on NativeAOT side 🤔

EgorBo marked this conversation as resolved.
Show resolved Hide resolved
}
#endif

// Ignore frozen objects
return WBF_NoBarrier;
}
Expand Down
14 changes: 14 additions & 0 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17502,6 +17502,20 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b
break;
}

case GT_CNS_INT:
{
if (tree->IsIconHandle(GTF_ICON_OBJ_HDL))
{
objClass = info.compCompHnd->getObjectType((void*)tree->AsIntCon()->IconValue());
if (objClass != NO_CLASS_HANDLE)
{
// if we managed to get a class handle it's definitely not null
*pIsNonNull = true;
}
}
break;
}

case GT_RET_EXPR:
{
// If we see a RET_EXPR, recurse through to examine the
Expand Down
Loading