Update the JIT to recognize the IsSupported property for all HWIntrinsics #23751

tannergooding · 2019-04-04T23:09:25Z

tannergooding · 2019-04-04T23:11:42Z

...stem.Private.CoreLib/shared/System/Runtime/Intrinsics/Arm/Arm64/Base.PlatformNotSupported.cs

@@ -15,7 +15,7 @@ namespace System.Runtime.Intrinsics.Arm.Arm64
    [CLSCompliant(false)]
    public static class Base
    {
-        public static bool IsSupported { get { return false; }}


An alternative to marking these explicitly as Intrinsic would be to mark all methods under System.Runtime.Intrinsics as [Intrinsic] and then have the importer create the PNSE node for unsupported ones.

One benefit of doing that would be that we wouldn't need the separate *.PlatformNotSupported.cs files for every ISA. The downside would be that you would get a stack overflow on runtimes that don't recognize the intrinsics at all...

I think that it makes the most sense to just mark the IsSupported ones. Otherwise, rather than the stack overflow behavior, I think we'd have to make it a requirement that all compilers on all platforms generate a throw to PNSE for any unrecognized System.Runtime.Intrinsics method, and I don't think that would be the right direction to go.

I think we'd have to make it a requirement that all compilers on all platforms generate a throw to PNSE for any unrecognized System.Runtime.Intrinsics method, and I don't think that would be the right direction to go.

I'm not so sure, I think that's the contract we have already defined for the HWIntrinsics and one that we plan on documenting. But, I think it can also be a separate discussion and shouldn't block the PR (I also don't feel particularly strongly about it).

tannergooding · 2019-04-04T23:12:30Z

src/jit/hwintrinsicArm64.cpp

@@ -101,45 +94,45 @@ InstructionSet Compiler::lookupHWIntrinsicISA(const char* className)
 //
 // Return Value:
 //    Id for the hardware intrinsic.
-//
-// TODO-Throughput: replace sequential search by hash lookup
 NamedIntrinsic Compiler::lookupHWIntrinsic(const char* className, const char* methodName)


This is basically identical to the x86 version now; but I wanted to just merge the HWIntrinsicInfo code in general in a follow up PR; rather than doing it piecemeal.

tannergooding · 2019-04-04T23:13:43Z

src/jit/importer.cpp

@@ -3458,6 +3458,21 @@ GenTree* Compiler::impIntrinsic(GenTree*                newobjThis,
            ni = lookupNamedIntrinsic(method);

 #ifdef FEATURE_HW_INTRINSICS
+            if (ni == NI_IsSupported_True)


We might be able to make this more general and move it out of FEATURE_HW_INTRINSICS; so we have a single spot for methods which are statically true/false...

What other methods do you have in mind?

A few are probably: BitConverter.IsLittleEndian, Environment.Is64BitProcess, and Vector.IsHardwareAccelerated which rely on other explicit recognition or JIT tricks.

There are likely others already and more to exist in the future as well.... The return gtNewIconNode(true); code isn't particular complex; but it might be nice to just be able to do the check and return NI_IsSupported_True or NI_IsSupported_False from lookupNamedIntrinsic; rather than having multiple separate paths that all do the same thing.

tannergooding · 2019-04-04T23:16:13Z

Validated both via the existing tests and locally by looking at the disassembly that this is working as expected on x86/x64.

What is the AzDO command for running the HWIntrinsic outer loop tests so I can also validate in CI?

tannergooding · 2019-04-05T01:00:23Z

Several of the test failures are due to https://github.com/dotnet/coreclr/issues/23730

tannergooding · 2019-04-05T01:02:11Z

ARM failures are mostly due to a missing import for the [Intrinsic] attribute in the x86 PlatformNotSupported.cs file; will have that resolved shortly.

CarolEidt

I didn't realize that this would be as straightforward as this - thanks @tannergooding !

CarolEidt · 2019-04-05T00:53:52Z

...stem.Private.CoreLib/shared/System/Runtime/Intrinsics/Arm/Arm64/Base.PlatformNotSupported.cs

@@ -15,7 +15,7 @@ namespace System.Runtime.Intrinsics.Arm.Arm64
    [CLSCompliant(false)]
    public static class Base
    {
-        public static bool IsSupported { get { return false; }}


I think that it makes the most sense to just mark the IsSupported ones. Otherwise, rather than the stack overflow behavior, I think we'd have to make it a requirement that all compilers on all platforms generate a throw to PNSE for any unrecognized System.Runtime.Intrinsics method, and I don't think that would be the right direction to go.

CarolEidt · 2019-04-05T00:58:30Z

src/jit/importer.cpp

@@ -3458,6 +3458,21 @@ GenTree* Compiler::impIntrinsic(GenTree*                newobjThis,
            ni = lookupNamedIntrinsic(method);

 #ifdef FEATURE_HW_INTRINSICS
+            if (ni == NI_IsSupported_True)


What other methods do you have in mind?

CarolEidt · 2019-04-05T00:59:35Z

src/jit/importer.cpp

    if ((namespaceName == nullptr) || (className == nullptr) || (methodName == nullptr))
    {
-        return result;
+        JITDUMP("Not recognized, not enough metadata\n");
+        return NI_Illegal;


Excellent; this needed more info in the dump - thanks!

CarolEidt · 2019-04-05T01:08:31Z

What is the AzDO command for running the HWIntrinsic outer loop tests so I can also validate in CI?

I can't find it. I looked on the AzDO builds page here: https://dev.azure.com/dnceng/public/_build?definitionId=98 but I don't see the hw intrinsics testing modes. @echesakovMSFT do we have them?

jkotas · 2019-04-05T01:08:52Z

Could you please explain rationale for doing this? Why the current structure does not work? (The change does not look like an improvement to me from what is mentioned here.)

echesakov · 2019-04-05T01:20:49Z

@CarolEidt @tannergooding

What is the AzDO command for running the HWIntrinsic outer loop tests so I can also validate in CI?

I can't find it. I looked on the AzDO builds page here: https://dev.azure.com/dnceng/public/_build?definitionId=98 but I don't see the hw intrinsics testing modes. @echesakovMSFT do we have them?

No we don't have HWIntrinsics-related build definitions in AzDO yet

But I have ported all the definitions of the Jenkins scenarios

coreclr/tests/testenvironment.proj

Lines 82 to 91 in 126aaf4

    
           <TestEnvironment Include="jitsse2only" EnableAVX="0" EnableSSE3_4="0" /> 
        
           <TestEnvironment Include="jitnosimd" FeatureSIMD="0" /> 
        
           <TestEnvironment Include="jitincompletehwintrinsic" EnableIncompleteISAClass="1" /> 
        
           <!-- testing the legacy SSE encoding --> 
        
           <TestEnvironment Include="jitx86hwintrinsicnoavx" EnableIncompleteISAClass="1" EnableAVX="0" /> 
        
           <!-- testing SNB/IVB --> 
        
           <TestEnvironment Include="jitx86hwintrinsicnoavx2" EnableIncompleteISAClass="1" EnableAVX2="0" /> 
        
           <!-- match "jitnosimd", may need to remove after decoupling HW intrinsic from FeatureSIMD --> 
        
           <TestEnvironment Include="jitx86hwintrinsicnosimd" EnableIncompleteISAClass="1" FeatureSIMD="0" /> 
        
           <TestEnvironment Include="jitnox86hwintrinsic" EnableIncompleteISAClass="1" EnableSSE="0" EnableSSE2="0" EnableSSE3="0" EnableSSSE3="0" EnableSSE41="0" EnableSSE42="0" EnableAVX="0" EnableAVX2="0" EnableAES="0" EnableBMI1="0" EnableBMI2="0" EnableFMA="0" EnableLZCNT="0" EnablePCLMULQDQ="0" EnablePOPCNT="0" />

so it should be pretty straighforward to add one more AzDO build definition and enable running those scenarios
/cc @RussKeldorph

tannergooding · 2019-04-05T01:40:27Z

Could you please explain rationale for doing this? Why the current structure does not work? (The change does not look like an improvement to me from what is mentioned here.)

@jkotas, this resolves https://github.com/dotnet/coreclr/issues/20379. We had an existing problem that the JIT would not always eliminate all the code from bool IsSupported => false;, so there was bits of codegen left about from doing if (Arm64.Simd.IsSupported) on x86 or if (X86.Sse.IsSupported) on ARM.

This just centralizes things so that the Isa.IsSupported checks are always treated as intrinsic; no matter what platform you are running on. It also removes all the duplicated IsSupported entries from the x86 table which reduces the number of entries that need to be checked on lookup. Finally itmoves the x86 and ARM name lookup code to be consistent, which will allow us to share more code between the two implementations in the future (most of this is trivially table driven, so sharing logic is readily possible in a number of cases).

jkotas · 2019-04-05T01:51:43Z

We had an existing problem that the JIT would not always eliminate all the code from bool IsSupported => false;, so there was bits of codegen left about from doing if (Arm64.Simd.IsSupported) on x86 or if (X86.Sse.IsSupported) on ARM.

Why is that? Having left-overs after a inlining a trivial method that returns false sounds pretty unfortunate. Isn't this something that should be better fixed somewhere else?

tannergooding · 2019-04-05T01:56:56Z

I asked a similar question here https://github.com/dotnet/coreclr/issues/20379#issuecomment-474484086 and JIT limits was brought up.

This aligned with other work that we wanted to do (more sharing of the lookup code), so put up the PR.

jkotas · 2019-04-05T02:56:03Z

ok. I guess that actual problem may be folks writing way too big methods with hardware intrintrisics that are hitting the limits. We should be splitting them into multiple methods per architecture, like here:

coreclr/src/System.Private.CoreLib/shared/System/Text/ASCIIUtility.cs

Line 556 in 2f4a756

? GetIndexOfFirstNonAsciiChar_Sse2(pBuffer, bufferLength)

tannergooding · 2019-04-05T14:53:57Z

We should be splitting them into multiple methods per architecture, like here

Right, and I believe that is the guidance we have been pushing and following. However, there are likely still cases where the branching and logic is itself fairly complex.

The default case is that users will have 3-4 paths: An x86/x64 accelerated path, a ARM/ARM64 accelerated path, and a Software Fallback path. Each architecture may then have individual ISA paths that it might take (such as SSE vs AVX or optional optimizations if SSE3 or SSE41 are available, etc). Some code may also be taking advantage of a Vector<T> code path (like our own code frequently does).

There are also cases where it isn't just a straightforward "exclusive" code path. The ASCIIUtility code you linked, for example, is a mix of software and vectorized code and itself already requires AggressiveInlining in some places just to get the JIT to cooperate given its complexity. The overall codegen ends up being decent overall, but explicitly treating the IsSupported=false methods as intrinsic will only help ensure that is the case moving forward.

…rted and ThrowPNSE

…ormNotSupported HWIntrinsic files

tannergooding · 2019-04-05T15:33:26Z

Rebased to pick up the latest test fixes from master.

tannergooding · 2019-04-05T17:46:41Z

ARM failures look unrelated. One is an assert in GC_Features._HeapExpansion_bestfit_1_bestfit_1_ the others are:

Interop_COM._NativeClients_Primitives_Primitives_._NativeClients_Primitives_Primitives_cmd
Interop_COM._NETClients_Primitives_NETClientPrimitives_NETClientPrimitives_._NETClients_Primitives_NETClientPrimitives_NETClientPrimitives_cmd
Interop_COM._NETClients_Primitives_NETClientPrimitivesInALC_NETClientPrimitivesInALC_._NETClients_Primitives_NETClientPrimitivesInALC_NETClientPrimitivesInALC_cmd
Interop_PInvoke._Miscellaneous_ThisCall_ThisCallTest_ThisCallTest_._Miscellaneous_ThisCall_ThisCallTest_ThisCallTest_cmd

echesakov · 2019-04-05T17:52:25Z

@tannergooding These failures are https://github.com/dotnet/coreclr/issues/22422 and https://github.com/dotnet/coreclr/issues/23577

tannergooding · 2019-04-05T19:56:13Z

@jkotas, do you have any pushback against this being merged?

jkotas · 2019-04-05T20:53:20Z

Looks fine to me.

…sics (dotnet/coreclr#23751) * Moving some JITDUMP calls into Compiler::lookupNamedIntrinsic * Marking the IsSupported methods as Intrinsic for all HWIntrinsic ISAs * Updating the hwintrinsic importation to more generally handle IsSupported and ThrowPNSE * Applying formatting patch. * Adding using System.Runtime.CompilerServices to the various x86 PlatformNotSupported HWIntrinsic files Commit migrated from dotnet/coreclr@1b4d7c7

tannergooding commented Apr 4, 2019

View reviewed changes

CarolEidt approved these changes Apr 5, 2019

View reviewed changes

tannergooding added 5 commits April 5, 2019 08:33

Moving some JITDUMP calls into Compiler::lookupNamedIntrinsic

07a6aff

Marking the IsSupported methods as Intrinsic for all HWIntrinsic ISAs

cb00495

Updating the hwintrinsic importation to more generally handle IsSuppo…

cf02924

…rted and ThrowPNSE

Applying formatting patch.

043ac18

Adding using System.Runtime.CompilerServices to the various x86 Platf…

cf56f20

…ormNotSupported HWIntrinsic files

tannergooding merged commit 1b4d7c7 into dotnet:master Apr 5, 2019

tannergooding mentioned this pull request Jun 11, 2019

Allow pregenerating most HW intrinsics in CoreLib #24917

Merged

jkotas mentioned this pull request Nov 2, 2020

Incomplete arm intrinsics substitions dotnet/runtime#44146

Closed

jkotas mentioned this pull request Oct 13, 2023

Add ILLink substitution for wasm intrinsics dotnet/runtime#93407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the JIT to recognize the IsSupported property for all HWIntrinsics #23751

Update the JIT to recognize the IsSupported property for all HWIntrinsics #23751

tannergooding commented Apr 4, 2019

tannergooding Apr 4, 2019

CarolEidt Apr 5, 2019

tannergooding Apr 5, 2019

tannergooding Apr 4, 2019

tannergooding Apr 4, 2019

CarolEidt Apr 5, 2019

tannergooding Apr 5, 2019

tannergooding commented Apr 4, 2019

tannergooding commented Apr 5, 2019

tannergooding commented Apr 5, 2019

CarolEidt left a comment

CarolEidt Apr 5, 2019

CarolEidt Apr 5, 2019

CarolEidt Apr 5, 2019

CarolEidt commented Apr 5, 2019

jkotas commented Apr 5, 2019 •

edited

Loading

echesakov commented Apr 5, 2019

tannergooding commented Apr 5, 2019

jkotas commented Apr 5, 2019

tannergooding commented Apr 5, 2019

jkotas commented Apr 5, 2019

tannergooding commented Apr 5, 2019 •

edited

Loading

tannergooding commented Apr 5, 2019

tannergooding commented Apr 5, 2019

echesakov commented Apr 5, 2019

tannergooding commented Apr 5, 2019

jkotas commented Apr 5, 2019

Update the JIT to recognize the IsSupported property for all HWIntrinsics #23751

Update the JIT to recognize the IsSupported property for all HWIntrinsics #23751

Conversation

tannergooding commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding commented Apr 4, 2019

tannergooding commented Apr 5, 2019

tannergooding commented Apr 5, 2019

CarolEidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarolEidt commented Apr 5, 2019

jkotas commented Apr 5, 2019 • edited Loading

echesakov commented Apr 5, 2019

tannergooding commented Apr 5, 2019

jkotas commented Apr 5, 2019

tannergooding commented Apr 5, 2019

jkotas commented Apr 5, 2019

tannergooding commented Apr 5, 2019 • edited Loading

tannergooding commented Apr 5, 2019

tannergooding commented Apr 5, 2019

echesakov commented Apr 5, 2019

tannergooding commented Apr 5, 2019

jkotas commented Apr 5, 2019

jkotas commented Apr 5, 2019 •

edited

Loading

tannergooding commented Apr 5, 2019 •

edited

Loading