JIT: assert compiling System.Runtime.Intrinsics.X86.Avx:TestC via PMI

[AndyAyersMS]

I made a couple of improvements to the generic expansions in PMI and am now seeing an assert when running PMI on system.private.corelib:

D:\repos\coreclr2\bin\tests\Windows_NT.x64.Checked\Tests\Core_Root\corerun.exe d:\repos\jitutils\bin\PMI.dll prepall-quiet D:\repos\coreclr2\bin\tests\Windows_NT.x64.Release\Tests\Core_Root\System.Private.CoreLib.dll

. . .

Assert failure(PID 3308 [0x00000cec], Thread: 36100 [0x8d04]): 
Assertion failed 'baseType != TYP_UNKNOWN' 
in 'System.Runtime.Intrinsics.X86.Avx:TestC(struct,struct):bool' (IL size 8)

    File: d:\repos\coreclr2\src\jit\hwintrinsicxarch.cpp Line: 728
    Image: D:\repos\coreclr2\bin\tests\Windows_NT.x64.Checked\Tests\Core_Root\CoreRun.exe

To repro, build x64 checked, sync to latest jitutils and build, then run the above.

[tannergooding]

@AndyAyersMS, how is PMI instantiating the generic types? The HWIntrinsics have JIT constraints that aren't expressed in the metadata type system.

[AndyAyersMS]

It uses reflection, so anything the runtime allows is fair game.... I can track down the exact instantiation if you like.

[tannergooding]

It uses reflection, so anything the runtime allows is fair game

That will cause problems for the HWIntrinsic types/methods. They are constrained in metadata to struct, but in the JIT to the 10 numeric primitive types (float, double, byte, sbyte, short, ushort, int, uint, long, and ulong`) with some methods being even more constrained (only supporting the integer types, for example).

[AndyAyersMS]

Ok. Looks like we are trying to instantiate with a Vector<float>.

What is the process for ultimately rejecting this kind of instantation? Is it left to the jit? If so, we should make sure when the type is invalid that the jit does not hit asserts first.

[tannergooding]

The logic starts here: https://github.com/dotnet/coreclr/blob/master/src/jit/hwintrinsicxarch.cpp#L636

The assert (L728) comes just before the first varTypeIsArithmetic check (which throws PNSE, if its an invalid type): https://github.com/dotnet/coreclr/blame/master/src/jit/hwintrinsicxarch.cpp#L736

[AndyAyersMS]

So presumably we can just remove the asserts above ... (for TYP_UNKNOWN and TYP_STRUCT)?

[AndyAyersMS]

Not quite sufficient... need something a bit more robust.

[tannergooding]

Yes, I believe so.

The only time it should fail is:

• The API is non-generic and we messed up somewhere
  • Codegen has its own asserts that should catch this type of issue
• The API is generic and the user provided an invalid type
  • We currently only use generic types when all 10 arithmetic types are supported
  • This should be handled by the PNSE checks right below

[AndyAyersMS]

(with asserts removed) - still see a TYP_UNKNOWN getting through:

Assert failure(PID 2532 [0x000009e4], Thread: 18676 [0x48f4]): Assertion failed '(type >= TYP_BYTE) && (type <= TYP_DOUBLE)' in 'System.Runtime.Intrinsics.X86.Avx:SetAllVector256(struct):struct' (IL size 7)

    File: d:\repos\coreclr2\src\jit\hwintrinsicxarch.h Line: 190
    Image: D:\repos\coreclr2\bin\tests\Windows_NT.x64.checked\Tests\Core_Root\CoreRun.exe

[tannergooding]

Hmmm... It seems to be missing out on the PNSE exception and is getting down to lookupIns on L767...

Let me build locally and try to step through here

[AndyAyersMS]

I had coded up this but perhaps it is too strong. Anyways I'll let you sort it out.

Basic point is that we should not assert if we're going to throw later, and we should always throw for the invalid cases.

@@ -721,11 +721,18 @@ GenTree* Compiler::impHWIntrinsic(NamedIntrinsic        intrinsic,
             if (baseType == TYP_UNKNOWN) // the second argument is not a vector
             {
                 baseType = JITtype2varType(strip(info.compCompHnd->getArgType(sig, secondArg, &secondArgClass)));
-                assert(baseType != TYP_STRUCT);
+
+                if (baseType == TYP_STRUCT)
+                {
+                    return impUnsupportedHWIntrinsic(CORINFO_HELP_THROW_TYPE_NOT_SUPPORTED, method, sig, mustExpand);
+                }
             }
         }
+    }

-        assert(baseType != TYP_UNKNOWN);
+    if (baseType == TYP_UNKNOWN)
+    {
+        return impUnsupportedHWIntrinsic(CORINFO_HELP_THROW_TYPE_NOT_SUPPORTED, method, sig, mustExpand);
     }

     if ((HWIntrinsicInfo::IsOneTypeGeneric(intrinsic) || HWIntrinsicInfo::IsTwoTypeGeneric(intrinsic)) &&

[tannergooding]

@AndyAyersMS, was the second assert happening for TestC as well? I wasn't able to get it to repro...

[AndyAyersMS]

No, it was for System.Runtime.Intrinsics.X86.Avx:SetAllVector256(struct):struct.

[AndyAyersMS]

Seeing a similar kind of failure here:

Assert failure(PID 7228 [0x00001c3c], Thread: 17884 [0x45dc]): Assertion failed '(type >= TYP_BYTE) && (type <= TYP_DOUBLE)' in 'System.Runtime.Intrinsics.X86.Sse2:SetZeroVector128():struct' (IL size 6)

    File: d:\repos\coreclr2\src\jit\hwintrinsicxarch.h Line: 190
    Image: D:\repos\coreclr2\bin\tests\Windows_NT.x64.Checked\Tests\Core_Root\CoreRun.exe

Would be great if we can PMI system.private.corelib cleanly with a checked jit....

Should I open a new issue?

[tannergooding]

@AndyAyersMS, a new issue would be great. I'll start working on this now, however.