Skip to content

Improve static field access for NativeAOT #63620

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

Closed
MichalStrehovsky wants to merge 1 commit into from
Closed

Improve static field access for NativeAOT #63620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions src/coreclr/inc/corinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1649,6 +1649,7 @@ enum CORINFO_FIELD_ACCESSOR
CORINFO_FIELD_STATIC_ADDR_HELPER, // static field accessed using address-of helper (argument is FieldDesc *)
CORINFO_FIELD_STATIC_TLS, // unmanaged TLS access
CORINFO_FIELD_STATIC_READYTORUN_HELPER, // static field access using a runtime lookup helper
CORINFO_FIELD_STATIC_DATASEGMENT, // static field access from the data segment
Copy link
Contributor

@SingleAccretion SingleAccretion Jan 11, 2022

Choose a reason for hiding this comment

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

We have documentation above on what the various accessors mean, it would be nice to update it to cover this new value.

(I realize some of it is rather outdated)


CORINFO_FIELD_INTRINSIC_ZERO, // intrinsic zero (IntPtr.Zero, UIntPtr.Zero)
CORINFO_FIELD_INTRINSIC_EMPTY_STRING, // intrinsic emptry string (String.Empty)
Expand Down
31 changes: 31 additions & 0 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8231,6 +8231,35 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT
}
break;

case CORINFO_FIELD_STATIC_DATASEGMENT:
Copy link
Contributor

@SingleAccretion SingleAccretion Jan 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This does look a lot like getFieldAddress with the second argument not set to a dummy value.

I realize it won't work as-is because the Jit assumes getFieldAddress will always return a direct address today (it should not be hard to fix that), it just does not feel great to have the duplicated functionality.

Edit: on second thought, plumbing this through getFieldAddress is not that great as it will require getFieldAddress to become more like getBaseFieldAddress. So I agree a new accessor is the better way to have things.

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Jan 12, 2022

Choose a reason for hiding this comment

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

The difference from getFieldAddress is that this gives RyuJIT a base+offset view instead of giving a single symbol that RyuJIT can't do much about (getFieldAddress returns the address with the base factored in).

It was the feedback from dotnet/corert#5131 (comment) (my earlier attempt) that ended up going the getFieldAddress path.

{
#ifdef FEATURE_READYTORUN
assert(opts.IsReadyToRun());
assert((pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_INITCLASS) == 0);

if (pFieldInfo->fieldLookup.accessType == InfoAccessType::IAT_VALUE)
{
op1 = gtNewIconHandleNode((size_t)pFieldInfo->fieldLookup.addr, GTF_ICON_STATIC_HDL);
op1->gtType = TYP_BYREF;
}
else
{
assert(pFieldInfo->fieldLookup.accessType == InfoAccessType::IAT_PVALUE);
op1 = gtNewIndOfIconHandleNode(TYP_BYREF, (size_t)pFieldInfo->fieldLookup.addr, GTF_ICON_GLOBAL_PTR, false);
Copy link
Contributor

@SingleAccretion SingleAccretion Jan 11, 2022

Choose a reason for hiding this comment

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

GTF_ICON_GLOBAL_PTR, isInvariant: false

What is the scenario where the address of the static can change?

Note that some code in the Jit depends on the addresses of statics being invariant. It will not matter for this change now, I believe, but I have changes in progress that will make this dependency more visible.

(To be clear: mutable addresses can absolutely be made to work, I just need to understand the scenarios I will need to add as tests to account for them)

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Jan 12, 2022

Choose a reason for hiding this comment

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

gtNewIndOfIconHandleNode asserts if GTF_ICON_GLOBAL_PTR is used with isInvariant = true. This was just me hammering on it until it worked. Maybe we need a new GTF_ value for this? I'm not particularly experienced in the RyuJIT code base; I know just enough to CTRL-C/CTRL-V from various places.

Copy link
Contributor

@SingleAccretion SingleAccretion Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, a new handle kind would be the best solution (GTF_STATIC_ADDR_PTR say...). Using GTF_ICON_CONST_PTR will work too but is less desirable. When adding a new handle kind, please also add printing for it to cTreeFlags and gtDispConst.

Why a new handle kind?

A "raw" GTF_ICON_CONST_PTR will work as an invariant indirection, but a dedicated handle kind will allow VN to treat it specially (and more precisely):

// We will have staticPtr = IND(STATIC_ADDR_PTR) and give "staticPtr" a "VNF_PtrToStatic" VN.
ref var staticPtr = ref staticField;

staticPtr = { ... }; // The compiler will be able to see that this store is just "staticField = { ... }"

It will be very similar to GTF_ICON_STATIC_BOX_PTR.

}

FieldSeqNode* fs = GetFieldSeqStore()->CreateSingleton(pResolvedToken->hField);
op1 = gtNewOperNode(GT_ADD, TYP_BYREF, op1,
new (this, GT_CNS_INT) GenTreeIntCon(TYP_INT, pFieldInfo->offset, fs));

// Prevent CSE from adding the offset to the base
op1->gtFlags |= GTF_DONT_CSE;
Comment on lines +8255 to +8256
Copy link
Contributor

@SingleAccretion SingleAccretion Jan 11, 2022

Choose a reason for hiding this comment

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

Why prevent CSE unconditionally? At least for the IAT_PVALUE case it seems it could be valuable, for the IAT_VALUE less so I suppose, except on ARM/64.

Is the issue here that we cannot fold Icon + Offset and end up CSEing it?
(If so would be worth a comment to that effect; seems like a bug in gtSetEvalOrder's logic for setting ADDRMODE_NO_CSE -- perhaps that's what the existing comment meant to say?)

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I was seeing RyuJIT fold the constant handle with the constant and that created an invalid handle (the handle is not an address when we're AOT compiling. It's an actual handle that should not be done constant propagation math on). Could be that this is a bug in CSE.

Copy link
Contributor

@SingleAccretion SingleAccretion Jan 12, 2022

Choose a reason for hiding this comment

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

I see. I think I know what the issue is (it should be a VN + assertion propagation bug). Could you open a bug on this and add a TODO-Bug: workaround for <link> here?

#else
unreached();
#endif // FEATURE_READYTORUN
}
break;

default:
{
// Do we need the address of a static field?
Expand Down Expand Up @@ -15567,6 +15596,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
case CORINFO_FIELD_STATIC_SHARED_STATIC_HELPER:
case CORINFO_FIELD_STATIC_GENERICS_STATIC_HELPER:
case CORINFO_FIELD_STATIC_READYTORUN_HELPER:
case CORINFO_FIELD_STATIC_DATASEGMENT:
op1 = impImportStaticFieldAccess(&resolvedToken, (CORINFO_ACCESS_FLAGS)aflags, &fieldInfo,
lclTyp);
break;
Expand Down Expand Up @@ -15838,6 +15868,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
case CORINFO_FIELD_STATIC_SHARED_STATIC_HELPER:
case CORINFO_FIELD_STATIC_GENERICS_STATIC_HELPER:
case CORINFO_FIELD_STATIC_READYTORUN_HELPER:
case CORINFO_FIELD_STATIC_DATASEGMENT:
op1 = impImportStaticFieldAccess(&resolvedToken, (CORINFO_ACCESS_FLAGS)aflags, &fieldInfo,
lclTyp);
break;
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1118,6 +1118,7 @@ public enum CORINFO_FIELD_ACCESSOR
CORINFO_FIELD_STATIC_ADDR_HELPER, // static field accessed using address-of helper (argument is FieldDesc *)
CORINFO_FIELD_STATIC_TLS, // unmanaged TLS access
CORINFO_FIELD_STATIC_READYTORUN_HELPER, // static field access using a runtime lookup helper
CORINFO_FIELD_STATIC_DATASEGMENT, // static field access from the data segment

CORINFO_FIELD_INTRINSIC_ZERO, // intrinsic zero (IntPtr.Zero, UIntPtr.Zero)
CORINFO_FIELD_INTRINSIC_EMPTY_STRING, // intrinsic emptry string (String.Empty)
Expand Down
35 changes: 27 additions & 8 deletions src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1921,6 +1921,7 @@ private void getFieldInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_MET
{
fieldFlags |= CORINFO_FIELD_FLAGS.CORINFO_FLG_FIELD_STATIC;

CORINFO_FIELD_ACCESSOR intrinsicAccessor;
if (field.HasRva)
{
fieldFlags |= CORINFO_FIELD_FLAGS.CORINFO_FLG_FIELD_UNMANAGED;
Expand Down Expand Up @@ -1981,20 +1982,38 @@ private void getFieldInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_MET
pResult->fieldLookup = CreateConstLookupToSymbol(helper);
}
}
else if (field.IsIntrinsic &&
(flags & CORINFO_ACCESS_FLAGS.CORINFO_ACCESS_GET) != 0 &&
(intrinsicAccessor = getFieldIntrinsic(field)) != (CORINFO_FIELD_ACCESSOR)(-1))
{
fieldAccessor = intrinsicAccessor;
}
else if (!field.IsThreadStatic && !_compilation.HasLazyStaticConstructor(field.OwningType))
{
fieldAccessor = CORINFO_FIELD_ACCESSOR.CORINFO_FIELD_STATIC_DATASEGMENT;

ISymbolNode baseAddress;
if (field.HasGCStaticBase)
{
pResult->fieldLookup.accessType = InfoAccessType.IAT_PVALUE;
baseAddress = _compilation.NodeFactory.TypeGCStaticsSymbol((MetadataType)field.OwningType);
}
else
{
pResult->fieldLookup.accessType = InfoAccessType.IAT_VALUE;
baseAddress = _compilation.NodeFactory.TypeNonGCStaticsSymbol((MetadataType)field.OwningType);
}

pResult->fieldLookup.addr = (void*)ObjectToHandle(baseAddress);
}
else
{
fieldAccessor = CORINFO_FIELD_ACCESSOR.CORINFO_FIELD_STATIC_SHARED_STATIC_HELPER;
pResult->helper = CorInfoHelpFunc.CORINFO_HELP_READYTORUN_STATIC_BASE;

ReadyToRunHelperId helperId = ReadyToRunHelperId.Invalid;
CORINFO_FIELD_ACCESSOR intrinsicAccessor;
if (field.IsIntrinsic &&
(flags & CORINFO_ACCESS_FLAGS.CORINFO_ACCESS_GET) != 0 &&
(intrinsicAccessor = getFieldIntrinsic(field)) != (CORINFO_FIELD_ACCESSOR)(-1))
{
fieldAccessor = intrinsicAccessor;
}
else if (field.IsThreadStatic)

if (field.IsThreadStatic)
{
helperId = ReadyToRunHelperId.GetThreadStaticBase;
}
Expand Down