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

[mono] Generate valid LLVM IR for vectors with unsupported element types #53132

Closed
wants to merge 2 commits into from

Conversation

imhameed
Copy link
Contributor

@imhameed imhameed commented May 23, 2021

Before this change, vectors with unsupported element types generated generic
"value type" code, which can cause invalid IR to be generated when compiling
System.Runtime.Intrinsics.WithLower<bool> on arm64. For posterity, here's the
relevant part of WithLower:

        public static Vector128<T> WithLower<T>(this Vector128<T> vector, Vector64<T> value)
            where T : struct
        {
            if (AdvSimd.IsSupported)
            {
                return AdvSimd.InsertScalar(vector.AsUInt64(), 0, value.AsUInt64()).As<ulong, T>();
            }
            ...

Vector128.As is currently implemented as an intrinsic that emits a bitcast,
but the Vector128<bool> and Vector64<bool> parameters are never explicitly
loaded from a pair of GPRs into vectors in the function prologue.

Before this change, WithLower<bool> generates:

define dso_local monocc i128 @"System.Runtime.Intrinsics.Vector128:WithLower<bool> (System.Runtime.Intrinsics.Vector128`1<bool>,System.Runtime.Intrinsics.Vector64`1<bool>)"([2 x i64] %arg_vector, [1 x i64] %arg_value) #0 gc "coreclr" {
BB0:
  %0 = bitcast [2 x i64] %arg_vector to <2 x i64>
  %1 = bitcast [1 x i64] %arg_value to i64
  %xinsert = insertelement <2 x i64> %0, i64 %1, i32 0
  %2 = bitcast <2 x i64> %xinsert to i128
  %3 = load i8 (i64*, i64*)*, i8 (i64*, i64*)** @"[tramp_6039] System.Type:op_Inequality (System.Type,System.Type)", align 8
  %4 = load i64, i64* inttoptr (i64 281473683542664 to i64*)
  %5 = icmp eq i64 %4, 0
  br i1 %5, label %gc.safepoint_poll.exit, label %gc.safepoint_poll.poll.i, !prof !1
  ...

The two bitcasts from arrays to first-class types are invalid. This would cause
an assertion failure if Mono is linked against a copy of LLVM built with
assertions enabled. With assertions disabled, this eventually causes opt/llc to
crash. This change works around this by generating undef values on function
entry for parameters that have SIMD-with-invalid-element types and associating
these dummy values to the mini IR registers that would be consumed by any
subsequent SIMD operations.

Future work:

FullAOT tests should be resurrected in dotnet/runtime.

The interaction between gsharedvt and SIMD types should be clarified.

@imhameed imhameed changed the title Generate valid LLVM IR for vectors with unsupported element types. [mono] Generate valid LLVM IR for vectors with unsupported element types. May 23, 2021
@imhameed imhameed added the runtime-mono specific to the Mono runtime label May 23, 2021
@imhameed
Copy link
Contributor Author

The runtime-staging failures look relevant.

@@ -860,7 +862,7 @@ mono_class_create_generic_inst (MonoGenericClass *gclass)
if (mono_is_corlib_image (gklass->image) &&
(!strcmp (gklass->name, "Vector`1") || !strcmp (gklass->name, "Vector64`1") || !strcmp (gklass->name, "Vector128`1") || !strcmp (gklass->name, "Vector256`1"))) {
MonoType *etype = gclass->context.class_inst->type_argv [0];
if (mono_type_is_primitive (etype) && etype->type != MONO_TYPE_CHAR && etype->type != MONO_TYPE_BOOLEAN)
if (!mini_is_gsharedvt_type (etype))
klass->simd_type = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

metadata/ code should not include mini.h, this should be done some other way.

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 also think this wouldn't completely fix Full AOT on arm64 because System.Runtime.Intrinsics.WithLower will be instantiated with T_GSHAREDVT, leading to exactly the same problem this was intended to fix; will check now and think about this a little more

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it can check klass->simd_type && mini_is_gsharedvt_type (..) in the jit, and fail gsharedvt, using GSHAREDVT_FAILURE ().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried another approach.

@imhameed imhameed added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 26, 2021
@imhameed imhameed requested a review from SamMonoRT as a code owner May 27, 2021 14:54
@imhameed imhameed changed the title [mono] Generate valid LLVM IR for vectors with unsupported element types. [mono] Generate valid LLVM IR for vectors with unsupported element types and handle return values for diverging functions May 27, 2021
@imhameed imhameed force-pushed the invalidvectors branch 2 times, most recently from 56a21e7 to ff5e097 Compare May 28, 2021 00:04
@runfoapp runfoapp bot mentioned this pull request May 28, 2021
imhameed added a commit that referenced this pull request Jun 2, 2021
…unctions. (#53536)

Dump optimized IR before attempting compilation to machine code. Makes
more information available when debugging.

Split from #53132.
imhameed added a commit that referenced this pull request Jun 2, 2021
Some functions never associate the mini IR source registers used with their
`setret` instructions with LLVM values because they unconditionally throw.

`System.Runtime.Intrinsics.X86.Aes:Decrypt` is an example of such a function;
when compiled for arm64, it never returns a value and unconditionally throws a
`PlatformNotSupportedException`.

We already had support for handling this for some, but not all, return value
passing conventions. Deduplicate this support and apply it uniformly to all
return value passing conventions that expect a populated mini IR source
register.

Make `LLVMArgNone` specifically mean "no return value"/"`void` return
type".

Split from #53132.

Partially fixes FullAOT compilation of System.Private.CoreLib.dll on arm64.
@imhameed imhameed changed the title [mono] Generate valid LLVM IR for vectors with unsupported element types and handle return values for diverging functions [mono] Generate valid LLVM IR for vectors with unsupported element types Jun 2, 2021
@tannergooding
Copy link
Member

tannergooding commented Jun 2, 2021

Why is the LLVM IR generated not simply the IL when the type isn't supported?

All of Vector<T> and Vector64/128/256<T> have some sort of IL that defines the "correct" behavior when an "unsupported" type is encountered and that is almost always throw new NotSupportedException()

I'd expect that the same happen here and so the LLVM IR should never get any vector IR for something like Vector<bool>.
-- For reference the supported types today are only: byte, sbyte, short, ushort, int, uint, long, ulong, double, and float (the 10 "core" primitive types).
-- Vector<T> also supports nint and nuint in .NET 6, support for these on Vector64/128/256<T> is approved but NYI
-- All other types should currently throw

@imhameed
Copy link
Contributor Author

imhameed commented Jun 4, 2021

Why is the LLVM IR generated not simply the IL when the type isn't supported?

All of Vector<T> and Vector64/128/256<T> have some sort of IL that defines the "correct" behavior when an "unsupported" type is encountered and that is almost always throw new NotSupportedException()

... Oh man. Thanks for the sanity check.

#53707

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Codegen-LLVM-mono NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) runtime-mono specific to the Mono runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants