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

[Arm64] Don't generate hardware intrinsic method bodies #39753

Merged
merged 3 commits into from
Jul 25, 2020

Conversation

echesakov
Copy link
Contributor

Fixes #39618

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 22, 2020
@echesakov echesakov force-pushed the Arm64-GitHub-39618 branch from 68c2af6 to 27a2165 Compare July 22, 2020 02:22
#if defined(TARGET_ARM64)
if (!fIsPlatformHWIntrinsic && fIsHWIntrinsic)
{
fTreatAsRegularMethodCall = (strcmp(className, "Vector64`1") != 0) && (strcmp(className, "Vector128`1") != 0);
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we need something similar for Vector128<T> and Vector256<T> on x86?

Copy link
Member

Choose a reason for hiding this comment

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

(I might just be missing where we cover it, if it exists somewhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding (that could be incorrect) is that on x86 in R2R code we want to treat all calls outside of System.Runtime.Intrinsics.X86 as regular calls since there is no guarantee what ISAs will be supported on target platform while on Arm64 - ArmBase and AdvSimd are baseline and always supported.

@davidwrighton Is it correct interpretation of discussion in #38060?

Copy link
Member

Choose a reason for hiding this comment

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

Ah right. I remember this reasoning now. It always confuses me at first 😄

#if defined(TARGET_ARM64)
if (!fIsPlatformHWIntrinsic && fIsHWIntrinsic)
{
fTreatAsRegularMethodCall = (strcmp(className, "Vector64`1") != 0) && (strcmp(className, "Vector128`1") != 0);
Copy link
Member

Choose a reason for hiding this comment

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

you are missing |=. It should be fTreatAsRegularMethodCall |= (strcmp(className, "Vector641") != 0) && (strcmp(className, "Vector1281") != 0);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I am not.

Consider this:

  1. If fTreatAsRegularMethodCall=true before line 2160, then it means fIsPlatformHWIntrinsic=true and the if-condition is false.
  2. If fTreatAsRegularMethodCall=false before line 2160, then fTreatAsRegularMethodCall = fTreatAsRegularMethodCall || (if-cond-expr) is equvalent to fTreatAsRegularMethodCall = (if-cond-expr).

I could've written this as

fTreatAsRegularMethodCall = (fIsGetIsSupportedMethod && fIsPlatformHWIntrinsic) || (!fIsPlatformHWIntrinsic && fIsHWIntrinsic 
#if defined(TARGET_ARM64)
    && ((strcmp(className, "Vector64`1") == 0) || (strcmp(className, "Vector128`1") == 0))
#endif
);

Copy link
Member

@kunalspathak kunalspathak Jul 22, 2020

Choose a reason for hiding this comment

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

I could've written this as

Yes either that or something like this:

        fTreatAsRegularMethodCall = (fIsGetIsSupportedMethod && fIsPlatformHWIntrinsic);

        if (!fIsPlatformHWIntrinsic && fIsHWIntrinsic)
        {
#if defined(TARGET_ARM64)
            // For ARM64, treat as method calls if HWIntrinsic is not the one supported.
            fTreatAsRegularMethodCall |= (strcmp(className, "Vector64`1") != 0) && (strcmp(className, "Vector128`1") != 0);
#else
            // For XArch, always treat HWIntrinsic as method calls.
  	    fTreatAsRegularMethodCall |= true;
#endif 
        }

Currently, it just feels like we override the fTreatAsRegularMethodCall without carefully inspecting the code the way you pointed.

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 pushed the simpler change. I think it's better to repeat (!fIsPlatformHWIntrinsic && fIsHWIntrinsic) twice.

@echesakov echesakov force-pushed the Arm64-GitHub-39618 branch from 27a2165 to 1f5e97e Compare July 24, 2020 21:28
@@ -450,7 +450,7 @@ void ZapInfo::CompileMethod()
}
#endif

#if defined(TARGET_X86) || defined(TARGET_AMD64)
#if defined(TARGET_X86) || defined(TARGET_AMD64) || defined(TARGET_ARM64)
if (methodAttribs & CORINFO_FLG_JIT_INTRINSIC)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AntonLapounov Can you please show me the place when I can disable pre-generation of the intrinsics bodies on Arm64 in CrossGen2?

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 CorInfoImpl.ShouldSkipCompilation controls that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks it already does this right

if (HardwareIntrinsicHelpers.IsHardwareIntrinsic(methodNeedingCode))
{
return true;
}

Thanks!

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

@jkotas jkotas merged commit 84eeebb into dotnet:master Jul 25, 2020
@echesakov echesakov deleted the Arm64-GitHub-39618 branch July 26, 2020 20:45
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
* Don't generate hardware intrinsic method bodies on Arm64 in zapinfo.cpp

* Treat Vector64 and Vector128 methods as intrinsics on Arm64 in zapinfo.cpp
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failure: JIT/HardwareIntrinsics/Arm/ArmBase.Arm64/ArmBase.Arm64_ro/ArmBase.Arm64_ro.sh
7 participants