Replace HELPER_METHOD_FRAME with DynamicHelperFrame in patchpoints#112025
Replace HELPER_METHOD_FRAME with DynamicHelperFrame in patchpoints#112025davidwrighton merged 10 commits intodotnet:mainfrom
Conversation
… and JIT_PartialCompilationPatchpoint - Consolidate the 2 functions so we only need 1 copy of the C++ code - Add asm helpers in the current officially supported architectures (The community ports will come later)
ghost
added
the
|
Tagging subscribers to this area: @mangod9 |
|
/azp run runtime-jit-experimental |
|
Azure Pipelines successfully started running 1 pipeline(s). |
davidwrighton
changed the title
am11
left a comment
am11
left a comment
There was a problem hiding this comment.
Looks great, thank you!
A side/post-HMF question, do we have some alternative/deterministic mechanism to replace the two VirtualUnwind calls in JIT_PatchpointWorkerWorkerWithPolicy?
| NESTED_END JIT_Patchpoint, _TEXT | ||
|
|
||
| ; first arg register holds iloffset, which needs to be moved to the second register, and the first register filled with NULL | ||
| LEAF_ENTRY JIT_PartialCompilationPatchpoint, _TEXT |
There was a problem hiding this comment.
Looks like we can remove the need of separateJIT_PartialCompilationPatchpoint if we update its only usage:
runtime/src/coreclr/jit/patchpoint.cpp
Lines 241 to 242 in d8a6117
to be:
GenTree* nullCounter = compiler->gtNewIconNode(0, TYP_I_IMPL);
GenTreeCall* helperCall =
compiler->gtNewHelperCallNode(CORINFO_HELP_PATCHPOINT, TYP_VOID, nullCounter, ilOffsetNode);
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
I likely would have done this differently. The head and tail portions are similar, the middle part has the policy, so the middle part could have been handled via two different auxiliary methods.
If we ever productize partial compilation there will likely be further changes in that policy, so keeping it separated would be nice.
|
jit-experimental has a known OSR failure #111625 which you are hitting, but the partial compilation failures are new to this PR. |
…to two easily distinguished functions.
|
Following @jkotas and @AndyAyersMS comments I've refactored the policy logic significantly. It looks much better to me now, and I hope it looks good to them too. I'll re-run the jit experimental leg if this looks good in regular PR tests. |
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
Thanks -- this looks good.
|
Could you please take at comments that are still open? |
|
@am11, about the calls to VirtualUnwind ... well, we could replace them by doing something with the TransitionBlock, but it would be a lot of processor specific bespoke code that would be easy to break. Until there is a perf issue, I'd like to just use the VirtualUnwind. |
|
/azp run runtime-jit-experimental |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jkotas
left a comment
jkotas
left a comment
There was a problem hiding this comment.
We like to mark methods that are only used within the file as static
Co-authored-by: Andy Ayers <andya@microsoft.com>
Merged wrong suggestion earlier
|
/azp run runtime-jit-experimental |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-jit-experimental |
|
Azure Pipelines successfully started running 1 pipeline(s). |
* main: (23 commits) add important remarks to NrbfDecoder (dotnet#111286) docs: fix spelling grammar and missing words in clr-code-guide.md (dotnet#112222) Consider type declaration order in MethodImpls (dotnet#111998) Add a feature flag to not use GVM in Linq Select (dotnet#109978) [cDAC] Implement ISOSDacInterface::GetMethodDescPtrFromIp (dotnet#110755) Restructure JSImport/JSExport generators to share more code and utilize more Microsoft.Interop.SourceGeneration shared code (dotnet#107769) Add more detailed explanations to control-flow RegexOpcode values (dotnet#112170) Add repo-specific condition to labeling workflows (dotnet#112169) Fix bad assembly when a nested exported type is marked via link.xml (dotnet#107945) Make `CalculateAssemblyAction` virtual. (dotnet#112154) JIT: Enable reusing profile-aware DFS trees between phases (dotnet#112198) Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory (dotnet#111877) JIT: Support custom `ClassLayout` instances with GC pointers in them (dotnet#112064) Factor positive lookaheads better into find optimizations (dotnet#112107) Add ImmutableCollectionsMarshal.AsMemory (dotnet#112177) [mono] ILStrip write directly to the output filestream (dotnet#112142) Allow the NativeAOT runtime pack to be specified as the ILC runtime package (dotnet#111876) JIT: some reworking for conditional escape analysis (dotnet#112194) Replace HELPER_METHOD_FRAME with DynamicHelperFrame in patchpoints (dotnet#112025) [Android] Decouple runtime initialization and entry point execution for Android sample (dotnet#111742) ...
4 participants
JIT_PatchpointandJIT_PartialCompilationPatchpointare still separate entrypoints, but the real meat of the logic is now all inJIT_PatchpointWorkerWorkerWithPolicy)DynamicHelperFramewhich appears to be sufficient.TransitionBlockand call into the common C++ code