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

Move arithmetic helpers to managed code #109087

Closed
wants to merge 36 commits into from

Conversation

am11
Copy link
Member

@am11 am11 commented Oct 21, 2024

Only the FCThrow ones in jithelpers have been moved. The JIT_GetRefAny method was left out due to the existing TODO regarding type inheritance. (JIT_GetRefAny is handled in #110344)

@am11 am11 force-pushed the feature/fcalls-cleanups branch from ac4bd28 to 3c98d26 Compare October 21, 2024 20:41
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 21, 2024
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 should check the perf impact on the affected architectures to see whether it is something we can live with.

There is alternatives with better perf possible: Inline the exception throwing in the JITed code (and optimize it out if the JIT can prove that it cannot happen). We do that on arm64 today.

@am11 am11 force-pushed the feature/fcalls-cleanups branch from c2bf9cc to 2a86f4d Compare October 24, 2024 12:01
@am11
Copy link
Member Author

am11 commented Oct 24, 2024

@AaronRobinsonMSFT, win-x86 leg is failing this assert:

Assert failure(PID 2788 [0x00000ae4], Thread: 3704 [0x0e78]): CONTRACT VIOLATION by CreateCOMPlusExceptionObject at "D:\a\_work\1\s\src\coreclr\vm\excep.cpp":5725

GC_TRIGGERS encountered in a GC_NOTRIGGER scope

                        CONTRACT in CreateCOMPlusExceptionObject at "D:\a\_work\1\s\src\coreclr\vm\excep.cpp":5725
                        CONTRACT in MethodDescCallSite::CallTargetWorker at "D:\a\_work\1\s\src\coreclr\vm\callhelpers.cpp":307
                        GCX_COOP in Assembly::ExecuteMainMethod at "D:\a\_work\1\s\src\coreclr\vm\assembly.cpp":1380
                        CONTRACT in Assembly::ExecuteMainMethod at "D:\a\_work\1\s\src\coreclr\vm\assembly.cpp":1366
                        GCX_COOP in CorHost2::ExecuteAssembly at "D:\a\_work\1\s\src\coreclr\vm\corhost.cpp":329
                        CONTRACT in CorHost2::ExecuteAssembly at "D:\a\_work\1\s\src\coreclr\vm\corhost.cpp":269

Does it mean we need to use something like FC_GC_POLL_RET besides FCALL_CONTRACT?

@jkotas
Copy link
Member

jkotas commented Oct 24, 2024

Does it mean we need to use something like FC_GC_POLL_RET

FC_GC_POLL_RET expands to HMF. It would not be an improvement.

I think the crash that you are seeing is caused by calling convention mismatch for 64-bit helpers. The existing helpers use VV flavor of the FCall macros, but your new helpers do not.

The existing NAOT implementation deals with the calling convention issues with using QCalls that use standard calling convention instead of managed calling convention.

@am11
Copy link
Member Author

am11 commented Dec 2, 2024

Software fallback impl was much worse: diff to base ratio is 61.50 vs. 1.58 with the FCall.

@am11
Copy link
Member Author

am11 commented Dec 5, 2024

@EgorBot -windows_intel -use32bit

using BenchmarkDotNet.Attributes;

public class Bench
{
    long a = 10000000000000000L;
    long b = 1000L;

    [Benchmark]
    public long SDiv() => a / b;
}

@am11
Copy link
Member Author

am11 commented Dec 5, 2024

Software fallback impl was much worse: diff to base ratio is 61.50 vs. 1.58 with the FCall.

With QCall+COMPlusThrow approach (f0cd111), it's 2.91. 🥲

@am11
Copy link
Member Author

am11 commented Jan 20, 2025

This is blocked until that half nanoseconds regression on x86 is sorted out on JIT side. Lets revisit this when that happens.

@am11 am11 closed this Jan 20, 2025
@am11 am11 deleted the feature/fcalls-cleanups branch January 20, 2025 13:13
@github-actions github-actions bot locked and limited conversation to collaborators Feb 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm32 arch-x86 area-VM-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants