Don't treat Math.Round, Math.Ceiling, or Math.Floor as intrinsics if we are pre-jitting. #25365

tannergooding · 2019-06-24T23:46:40Z

This resolves https://github.com/dotnet/corefx/issues/38762

tannergooding · 2019-06-24T23:50:19Z

The following are all the places where we currently use compSupports today:

src/jit/codegenarm64.cpp:2845:    if (compiler->compSupports(InstructionSet_Atomics))                                                                                                                                                                                                                
src/jit/codegenarm64.cpp:2982:    if (compiler->compSupports(InstructionSet_Atomics))                                                                                                                                                                                                                
src/jit/codegenxarch.cpp:7255:    assert(compiler->compSupports(InstructionSet_SSE41) && !IsPreJIT());                                                                                                                                                                                               
src/jit/compiler.cpp:2342:    if (jitFlags.IsSet(JitFlags::JIT_FLAG_USE_BMI1) && JitConfig.EnableBMI1() && compSupports(InstructionSet_AVX))                                                                                                                                                         
src/jit/compiler.cpp:2352:    if (jitFlags.IsSet(JitFlags::JIT_FLAG_USE_BMI2) && JitConfig.EnableBMI2() && compSupports(InstructionSet_AVX))                                                                                                                                                         
src/jit/compiler.h:7486:        if (compSupports(InstructionSet_AVX2))                                                                                                                                                                                                                               
src/jit/compiler.h:7491:        if (compSupports(InstructionSet_SSE42))                                                                                                                                                                                                                              
src/jit/compiler.h:7977:        if (compSupports(InstructionSet_AVX))                                                                                                                                                                                                                                
src/jit/compiler.h:8108:    bool compSupports(InstructionSet isa) const                                                                                                                                                                                                                              
src/jit/compiler.h:8120:        return compSupports(InstructionSet_AVX);                                                                                                                                                                                                                             
src/jit/gentree.cpp:16445:                if (compSupports(InstructionSet_SSE))                                                                                                                                                                                                                      
src/jit/gentree.cpp:16454:                if (compSupports(InstructionSet_AVX))                                                                                                                                                                                                                      
src/jit/hwintrinsicArm64.cpp:109:    bool isIsaSupported = compSupports(isa) && compSupportsHWIntrinsic(isa);                                                                                                                                                                                        
src/jit/hwintrinsiccodegenxarch.cpp:1252:    assert(compiler->compSupports(InstructionSet_SSE));                                                                                                                                                                                                     
src/jit/hwintrinsicxarch.cpp:61:    bool isIsaSupported = comp->compSupports(isa) && comp->compSupportsHWIntrinsic(isa);                                                                                                                                                                             
src/jit/hwintrinsicxarch.cpp:1064:            if (!compSupports(InstructionSet_AVX))                                                                                                                                                                                                                 
src/jit/hwintrinsicxarch.cpp:1122:            if (compSupports(InstructionSet_SSE2) || (compSupports(InstructionSet_SSE) && (baseType == TYP_FLOAT)))                                                                                                                                                
src/jit/hwintrinsicxarch.cpp:1134:            if (compSupports(InstructionSet_SSE) && varTypeIsFloating(baseType))                                                                                                                                                                                   
src/jit/hwintrinsicxarch.cpp:1148:            if (compSupports(InstructionSet_AVX))                                                                                                                                                                                                                  
src/jit/hwintrinsicxarch.cpp:1160:            if (compSupports(InstructionSet_SSE))                                                                                                                                                                                                                  
src/jit/hwintrinsicxarch.cpp:1181:            if (compSupports(InstructionSet_AVX))                                                                                                                                                                                                                  
src/jit/hwintrinsicxarch.cpp:1193:            if (compSupports(InstructionSet_AVX) && varTypeIsFloating(baseType))                                                                                                                                                                                   
src/jit/hwintrinsicxarch.cpp:1205:            if (compSupports(InstructionSet_AVX))                                                                                                                                                                                                                  
src/jit/hwintrinsicxarch.cpp:1214:            if (!compSupports(InstructionSet_AVX))                                                                                                                                                                                                                 
src/jit/hwintrinsicxarch.cpp:1226:            if (!compSupports(InstructionSet_SSE2) || !varTypeIsArithmetic(baseType) || !indexOp->OperIsConst())                                                                                                                                                   
src/jit/hwintrinsicxarch.cpp:1242:                    if (!compSupports(InstructionSet_SSE41))                                                                                                                                                                                                       
src/jit/hwintrinsicxarch.cpp:1250:                    if (!compSupports(InstructionSet_SSE41_X64))                                                                                                                                                                                                   
src/jit/hwintrinsicxarch.cpp:1287:                assert(compSupports(InstructionSet_AVX));                                                                                                                                                                                                          
src/jit/hwintrinsicxarch.cpp:1318:                    if (!compSupports(InstructionSet_SSE41))                                                                                                                                                                                                       
src/jit/hwintrinsicxarch.cpp:1440:            if (!compSupports(InstructionSet_AVX))                                                                                                                                                                                                                 
src/jit/hwintrinsicxarch.cpp:1452:            if (!compSupports(InstructionSet_SSE2) || !varTypeIsArithmetic(baseType) || !indexOp->OperIsConst())                                                                                                                                                   
src/jit/hwintrinsicxarch.cpp:1468:                    if (!compSupports(InstructionSet_SSE41))                                                                                                                                                                                                       
src/jit/hwintrinsicxarch.cpp:1476:                    if (!compSupports(InstructionSet_SSE41_X64))                                                                                                                                                                                                   
src/jit/hwintrinsicxarch.cpp:1508:                assert(compSupports(InstructionSet_AVX));                                                                                                                                                                                                          
src/jit/hwintrinsicxarch.cpp:1566:                    if (!compSupports(InstructionSet_SSE41))                                                                                                                                                                                                       
src/jit/importer.cpp:4051:                if (compSupports(InstructionSet_FMA))                                                                                                                                                                                                                      
src/jit/importer.cpp:19826:            return compSupports(InstructionSet_SSE41) && !IsPreJIT();                                                                                                                                                                                                     
src/jit/lowerarmarch.cpp:81:                return comp->compSupports(InstructionSet_Atomics) ? false                                                                                                                                                                                                
src/jit/lsraarm64.cpp:381:            if (!compiler->compSupports(InstructionSet_Atomics))                                                                                                                                                                                                           
src/jit/lsraarm64.cpp:403:                if (!compiler->compSupports(InstructionSet_Atomics))                                                                                                                                                                                                       
src/jit/lsraarm64.cpp:423:            if (!compiler->compSupports(InstructionSet_Atomics))                                                                                                                                                                                                           
src/jit/lsraarm64.cpp:443:            if (!compiler->compSupports(InstructionSet_Atomics))

I'm still going through these, but it looks like there may be a couple other places that need to be fixed up as well (this is ignoring any code path that uses the VEX encoding, as those are currently forced to false).

tannergooding · 2019-06-24T23:56:16Z

Went through the entries and updated a couple more places. Namely, the SIMD intrinsics use a "support level" to determine how they should be generated (this impacts Vector2/3/4) and the HWIntrinsics would generate nodes for some of the helper methods based on the compiler support.

tannergooding · 2019-06-25T00:01:17Z

src/jit/compiler.h

@@ -7466,6 +7466,11 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 #pragma endregion // Note: region is NOT under !defined(__GNUC__)
 #endif

+    bool IsPreJIT()
+    {
+        return jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT);


There are a lot of places in the JIT that just do jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT); directly, it might be useful to update those to call IsPreJIT() post the 3.0 snap.

jkotas · 2019-06-25T01:41:20Z

src/jit/compiler.h

@@ -8297,6 +8277,11 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
        }
 #endif

+        bool IsPreJIT()


Why do you need to check whether this is PreJIT?

This should be based solely on the available instruction set, no?

Ok, I see.

Would it be better to filter out the problematic methods in FilterHardwareIntrinsicMethodAttribs instead. I think it would be nice to have the hacks required to make the intrinsics in CoreLib work in a central place.

I went with this as I believed it to be the minimal set of changes needed for netcoreapp3.0 and would have the least impact on what code could be AOT'd.

If we are just talking about S.P.Corelib, I think we would only need to filter out the Math.Round, Math.Ceiling, and Math.Floor methods (as the other relevant changes apply to the Vector64/128/256<T> helper methods, which should already be getting filtered).

If we go a bit broader, this also impacts various codegen around FEATURE_SIMD with relation to the "SIMD support level" which can be AVX2, SSE41, or SSE2. At first glance, that mostly looks limited to the SIMD intrinsics (and therefore, outside of Vector<T>, to the System.Numerics library), but there is quite a bit of handling around those and I didn't go through it all.

If we did filter them in FilterHardwareIntrinsicMethodAttribs, that might also limit the AOT codegen quality if/when we expand the support beyond S.P.Corelib (but that could also be handled at that point in time.)

The strategy to expand to support outside CoreLib will need to be different. So I think it would be best to keep the hack required to make the CoreLib work centralized, and not spread it around.

Hmmm, Math.Ceiling and Math.Floor might be problematic. They are still FCALLs and so only get CORINFO_FLG_INTRINSIC. Math.Round, however, is a named intrinsic and so can go through the normal filtering.

Do we have a good way of allowing Math.Ceiling/Math.Floor to still be resolved to their CRT helper but not allowing the additional intrinsic handling?

In ZapInfo::getIntrinsicID, check the CORINFO_INTRINSIC_Ceiling / CORINFO_INTRINSIC_Floor and return CORINFO_INTRINSIC_Illegal instead.

CarolEidt · 2019-06-25T04:04:28Z

Overall this seems reasonable. Just to clarify/verify a couple of things:

The Vector128 changes are needed because these aren't ISA-specific intrinsics that would be protected in usage by IsSupported calls - rather, their codegen is determined by the execution platform, is that right?
I gather that, as @jkotas suggests, you will move some of the checking into the zapper.
The getSIMDSupportLevel change is necessary because, although we don't generate code for Vector<T> we do generate code for the fixed vector types Vector2, Vector3, and Vector4, and this code can be different for SSE4+ than for SSE2.

MichalStrehovsky · 2019-06-25T08:15:39Z

Thanks for looking into this and sorry for the trouble!

I like Jan's suggestion of filtering this in the zapper because it makes this more centralized - if we later add command line switches that people have been asking for ("assume the target ISA is XYZ and generate the best code for that"), we'll only have to pipe that switch within the zapper - RyuJIT won't have to learn about those kinds of IsPreJIT policies.

tannergooding · 2019-06-25T16:10:35Z

Just to clarify/verify a couple of things:

I believe @jkotas would prefer all of the checks to be routed through the zapper.

Having gone through the code that uses getSIMDSupportLevel, it looks like that will be fine to not do for S.P.CoreLib (as it appears getSIMDSupportLevel is currently only used for System.Numerics.Vectors.dll). We will definitely need to handle it post 3.0, and I don't think we can do that properly in FilterHardwareIntrinsicMethodAttribs under the current setup.

I believe the Vector128 changes are also unnecessary as FilterHardwareIntrinsicMethodAttribs is already marking those intrinsic calls as not-intrinsic (but I am confirming).

…endent on the ISAs supported by the target CPU.

tannergooding · 2019-06-25T17:51:45Z

Updated to be done exclusively in the zapper. That resulted in me renaming FilterHardwareIntrinsicMethodAttribs to FilterNamedIntrinsicMethodAttribs and to do some minor refactoring so it could more easily share the "treat as regular method call" stripping logic.

tannergooding · 2019-06-25T17:57:10Z

This does not touch getSIMDSupportLevel as called out in #25365 (comment)

jkotas

Thanks

CarolEidt

LGTM thanks! - it definitely seems cleaner to handle this in the zapper.

tannergooding · 2019-06-25T19:46:54Z

@MichalStrehovsky, could you give this a look as well?

MichalStrehovsky

Thanks for cleaning up the Filter method!

Has to be scoped out for now because of the reasons in dotnet/coreclr#25365. Can't apply the same strategy as in that pull request because we precompile `Vector<T>` here (we always JIT in CoreCLR).

…endent on the ISAs supported by the target CPU. (dotnet/coreclr#25365) Commit migrated from dotnet/coreclr@133e857

tannergooding requested review from MichalStrehovsky and CarolEidt June 25, 2019 00:00

tannergooding commented Jun 25, 2019

View reviewed changes

jkotas reviewed Jun 25, 2019

View reviewed changes

Updating the zapper to filter out some named intrinsics which are dep…

91a438f

…endent on the ISAs supported by the target CPU.

jkotas approved these changes Jun 25, 2019

View reviewed changes

CarolEidt approved these changes Jun 25, 2019

View reviewed changes

MichalStrehovsky approved these changes Jun 25, 2019

View reviewed changes

jkotas merged commit 133e857 into dotnet:master Jun 25, 2019

This was referenced Sep 26, 2019

[CPAOT] Generating code for hardware intrinsic #26772

Merged

[CPAOT] Generating code for hardware intrinsic (#26772) #26982

Closed

EgorBo mentioned this pull request Jan 31, 2020

Math.Round returns different values for the same input. dotnet/runtime#13144

Closed

This was referenced Apr 4, 2020

Vector128<byte>:get_Zero() doesn't inline (or intrinsicify) at crossgen dotnet/runtime#32714

Closed

FilterNamedIntrinsicMethodAttribs checks className for unsupported isas not classname dotnet/runtime#34826

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't treat Math.Round, Math.Ceiling, or Math.Floor as intrinsics if we are pre-jitting. #25365

Don't treat Math.Round, Math.Ceiling, or Math.Floor as intrinsics if we are pre-jitting. #25365

tannergooding commented Jun 24, 2019

tannergooding commented Jun 24, 2019

tannergooding commented Jun 24, 2019

tannergooding Jun 25, 2019

jkotas Jun 25, 2019

jkotas Jun 25, 2019

tannergooding Jun 25, 2019

tannergooding Jun 25, 2019

jkotas Jun 25, 2019

tannergooding Jun 25, 2019

jkotas Jun 25, 2019

CarolEidt commented Jun 25, 2019

MichalStrehovsky commented Jun 25, 2019

tannergooding commented Jun 25, 2019

tannergooding commented Jun 25, 2019

tannergooding commented Jun 25, 2019

jkotas left a comment

CarolEidt left a comment

tannergooding commented Jun 25, 2019

MichalStrehovsky left a comment

Don't treat Math.Round, Math.Ceiling, or Math.Floor as intrinsics if we are pre-jitting. #25365

Don't treat Math.Round, Math.Ceiling, or Math.Floor as intrinsics if we are pre-jitting. #25365

Conversation

tannergooding commented Jun 24, 2019

tannergooding commented Jun 24, 2019

tannergooding commented Jun 24, 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

Choose a reason for hiding this comment

CarolEidt commented Jun 25, 2019

MichalStrehovsky commented Jun 25, 2019

tannergooding commented Jun 25, 2019

tannergooding commented Jun 25, 2019

tannergooding commented Jun 25, 2019

jkotas left a comment

Choose a reason for hiding this comment

CarolEidt left a comment

Choose a reason for hiding this comment

tannergooding commented Jun 25, 2019

MichalStrehovsky left a comment

Choose a reason for hiding this comment