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.

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

CG2 Add lazy string literal loading support #47183

Closed
wants to merge 5 commits into from
Closed

CG2 Add lazy string literal loading support #47183

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9dd7fc0
CG2 Add lazy string literal loading support
nattress Jan 16, 2021
a29f31d
Move to CoreInfoImpl.ReadyToRun.cs
nattress Jan 19, 2021
30660cc
Add a lazy thunk for the GetString helper
nattress Jan 20, 2021
8a56491
Update src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorIn…
nattress Jan 20, 2021
84ade17
Add missing Arm64 reloc support
nattress Jan 26, 2021
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
4 changes: 2 additions & 2 deletions src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_X64/X64Emitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ public void EmitMOV(Register regDst, ISymbolNode node)
{
if (node.RepresentsIndirectionCell)
{
Builder.EmitByte(0x67);
Builder.EmitByte(0x48);
AddrMode rexAddrMode = new AddrMode(Register.RAX, null, 0, 0, AddrModeSize.Int64);
EmitRexPrefix(regDst, ref rexAddrMode);
Builder.EmitByte(0x8B);
Builder.EmitByte((byte)(0x00 | ((byte)regDst << 3) | 0x05));
Builder.EmitReloc(node, RelocType.IMAGE_REL_BASED_REL32);
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 @@ -2753,12 +2753,6 @@ private uint getThreadTLSIndex(ref void* ppIndirection)
private void getFunctionFixedEntryPoint(CORINFO_METHOD_STRUCT_* ftn, ref CORINFO_CONST_LOOKUP pResult)
{ throw new NotImplementedException("getFunctionFixedEntryPoint"); }

private CorInfoHelpFunc getLazyStringLiteralHelper(CORINFO_MODULE_STRUCT_* handle)
{
// TODO: Lazy string literal helper
return CorInfoHelpFunc.CORINFO_HELP_UNDEF;
}

private CORINFO_MODULE_STRUCT_* embedModuleHandle(CORINFO_MODULE_STRUCT_* handle, ref void* ppIndirection)
{ throw new NotImplementedException("embedModuleHandle"); }
private CORINFO_CLASS_STRUCT_* embedClassHandle(CORINFO_CLASS_STRUCT_* handle, ref void* ppIndirection)
Expand Down
8 changes: 8 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,9 @@ private ISymbolNode GetHelperFtnUncached(CorInfoHelpFunc ftnNum)
case CorInfoHelpFunc.CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT:
id = ReadyToRunHelper.ReversePInvokeExit;
break;
case CorInfoHelpFunc.CORINFO_HELP_STRCNS_CURRENT_MODULE:
id = ReadyToRunHelper.GetString;
Copy link
Member

Choose a reason for hiding this comment

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

I do not think it is as simple as this. There needs to be a code to add the current module argument for the helper (it is what ZapLazyHelperThunk does in crossgen1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I thought the CORINFO_HELP_STRCNS_CURRENT_MODULE meant we didn't need that. I'll look into adding that lazy thunk.

Copy link
Member

Choose a reason for hiding this comment

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

I think that ImportThunk already has a Lazy variant that I think was supposed to cater for lazy strings, among others. Having said that, it hasn't been tested until now so there's a high chance of it being broken in some way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Tomas - I corrected things to call the lazy thunk and fixed an x64 bug. I'll kick off a CG2 run to make sure we're okay on other architectures.

nattress marked this conversation as resolved.
Show resolved Hide resolved
return _compilation.NodeFactory.ImportThunk(ReadyToRunHelper.GetString, _compilation.NodeFactory.HelperImports, useVirtualCall: false);

case CorInfoHelpFunc.CORINFO_HELP_INITCLASS:
case CorInfoHelpFunc.CORINFO_HELP_INITINSTCLASS:
Expand All @@ -819,6 +822,11 @@ private ISymbolNode GetHelperFtnUncached(CorInfoHelpFunc ftnNum)
return _compilation.NodeFactory.GetReadyToRunHelperCell(id);
}

private CorInfoHelpFunc getLazyStringLiteralHelper(CORINFO_MODULE_STRUCT_* handle)
{
return CorInfoHelpFunc.CORINFO_HELP_STRCNS_CURRENT_MODULE;
Copy link
Member

Choose a reason for hiding this comment

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

Is this right for strings from other modules? Crossgen1 version of this method has a check for current module in this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that's a fragile ngen scenario. getLazyStringLiteralHelper gets called from the below handling code in the JIT. Returning CORINFO_HELP_STRCNS causes the JIT to call embedModuleHandle which is not supported in R2R.

runtime/src/coreclr/jit/morph.cpp

Lines 9287 to 9301 in 1390941

CorInfoHelpFunc helper = info.compCompHnd->getLazyStringLiteralHelper(tree->AsStrCon()->gtScpHnd);
if (helper != CORINFO_HELP_UNDEF)
{
// For un-important blocks, we want to construct the string lazily
GenTreeCall::Use* args;
if (helper == CORINFO_HELP_STRCNS_CURRENT_MODULE)
{
args = gtNewCallArgs(gtNewIconNode(RidFromToken(tree->AsStrCon()->gtSconCPX), TYP_INT));
}
else
{
args = gtNewCallArgs(gtNewIconNode(RidFromToken(tree->AsStrCon()->gtSconCPX), TYP_INT),
gtNewIconEmbScpHndNode(tree->AsStrCon()->gtScpHnd));
}

Copy link
Member

Choose a reason for hiding this comment

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

Does this all work well for large version bubble or composite mode cases?

}

private void getFunctionEntryPoint(CORINFO_METHOD_STRUCT_* ftn, ref CORINFO_CONST_LOOKUP pResult, CORINFO_ACCESS_FLAGS accessFlags)
{
throw new RequiresRuntimeJitException(HandleToObject(ftn).ToString());
Expand Down