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

Replace HELPER_METHOD_FRAME with DynamicHelperFrame in patchpoints #112025

Merged
merged 10 commits into from
Feb 5, 2025

Conversation

davidwrighton
Copy link
Member

@davidwrighton davidwrighton commented Jan 31, 2025

  • Consolidate the 2 functions so we only need 1 copy of the C++ code (JIT_Patchpoint and JIT_PartialCompilationPatchpoint are still separate entrypoints, but the real meat of the logic is now all in JIT_PatchpointWorkerWorkerWithPolicy)
  • Instead of using a managed function, I decided to use a transition frame to manage the case of calling into the runtime. In this case we are able to re-use the DynamicHelperFrame which appears to be sufficient.
  • Add asm helpers in the current architectures which support on stack replacement to setup the TransitionBlock and call into the common C++ code

… 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)
Copy link
Contributor

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

@davidwrighton
Copy link
Member Author

/azp run runtime-jit-experimental

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidwrighton davidwrighton changed the title [DRAFT] Replace HELPER_METHOD_FRAME with DynamicHelperFrame in patchpoints Replace HELPER_METHOD_FRAME with DynamicHelperFrame in patchpoints Jan 31, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 31, 2025
@davidwrighton davidwrighton marked this pull request as ready for review January 31, 2025 23:34
Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we can remove the need of separateJIT_PartialCompilationPatchpoint if we update its only usage:

GenTreeCall* helperCall =
compiler->gtNewHelperCallNode(CORINFO_HELP_PARTIAL_COMPILATION_PATCHPOINT, TYP_VOID, ilOffsetNode);

to be:

GenTree* nullCounter = compiler->gtNewIconNode(0, TYP_I_IMPL);
GenTreeCall* helperCall =
    compiler->gtNewHelperCallNode(CORINFO_HELP_PATCHPOINT, TYP_VOID, nullCounter, ilOffsetNode);

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

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.

src/coreclr/vm/jithelpers.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/jithelpers.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/jithelpers.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/jithelpers.cpp Show resolved Hide resolved
src/coreclr/vm/jithelpers.cpp Show resolved Hide resolved
@AndyAyersMS
Copy link
Member

jit-experimental has a known OSR failure #111625 which you are hitting, but the partial compilation failures are new to this PR.

@davidwrighton
Copy link
Member Author

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.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Thanks -- this looks good.

@jkotas
Copy link
Member

jkotas commented Feb 3, 2025

Could you please take at comments that are still open?

#112025 (review)
#112025 (review)

#112025 (comment)
#112025 (comment)

@davidwrighton
Copy link
Member Author

@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.

@davidwrighton
Copy link
Member Author

/azp run runtime-jit-experimental

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

We like to mark methods that are only used within the file as static

src/coreclr/vm/jithelpers.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/jithelpers.cpp Outdated Show resolved Hide resolved
Merged wrong suggestion earlier
@jkotas
Copy link
Member

jkotas commented Feb 5, 2025

/azp run runtime-jit-experimental

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@jkotas
Copy link
Member

jkotas commented Feb 5, 2025

/azp run runtime-jit-experimental

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidwrighton davidwrighton merged commit 5148d9a into dotnet:main Feb 5, 2025
104 of 108 checks passed
grendello added a commit to grendello/runtime that referenced this pull request Feb 6, 2025
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants