Skip to content

Simplify making IL base JIT Helpers - #108349

Merged
davidwrighton merged 5 commits into
dotnet:mainfrom
davidwrighton:il_helpers_perf_improvement
Oct 18, 2024
Merged

davidwrighton merged 5 commits into
dotnet:mainfrom
davidwrighton:il_helpers_perf_improvement

Conversation

@davidwrighton

Copy link
Copy Markdown
Member
  • Currently, for best performance, one needs to know to update a switch statement in jitinterface.cpp whenever a helper is moved to managed
  • Instead, with this change, if the helper has a specified managed method implementation, that check will just work
  • Also, if there is a native implementation, then the native implementation will take precedence. (This is to allow for helpers to have alternative assembly based implementations)

This should help fix a regression #108203 introduced with my recent work around generics lookups.

- Currently, for best performance, one needs to know to update a switch statement in jitinterface.cpp whenever a helper is moved to managed
- Instead, with this change, if the helper has a specified managed method implementation, that check will just work
- Also, if there is a native implementation, then the native implementation will take precedence. (This is to allow for helpers to have alternative assembly based implementations)

This should help fix a regression dotnet#108203 introduced with my recent work around generics lookups.
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@EgorBo

EgorBo commented Sep 27, 2024

Copy link
Copy Markdown
Member

@davidwrighton there is also this code that you might want to change

// Let's tier up all cast helpers faster than other methods. This is because we want to import them as
// direct calls in codegen and they need to be promoted earlier than their callers.
if (methodDesc->GetMethodTable() == g_pCastHelpers)
{
callCountThreshold = max<CallCount>(1, (CallCount)(callCountThreshold / 2));
}

so for this "indirect -> direct" optimization to kick in, especially in micro-benchmarks, it's better to promote such helpers to Tier1 earlier.

@davidwrighton

Copy link
Copy Markdown
Member Author

@EgorBo ... thanks for the heads up. I'll look into that later.

}
else
{
instGen_Set_Reg_To_Imm(EA_PTRSIZE, callTarget, (ssize_t)addr);

@EgorBo EgorBo Oct 18, 2024 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this could be just instGen_Set_Reg_To_Imm(EA_PTR_DSP_RELOC, ...) without if-else here. It will either use addr+add or just emit a 64bit constant inside the function itself. See its impl:

if (!compiler->opts.compReloc)
{
size = EA_SIZE(size); // Strip any Reloc flags from size if we aren't doing relocs
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fine to merge as is, just a nit

@davidwrighton
davidwrighton merged commit 1fa61b9 into dotnet:main Oct 18, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Nov 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants