Skip to content

Remove an assert from impHWIntrinsic - #72084

Merged
EgorBo merged 3 commits into
dotnet:mainfrom
EgorBo:fix-bogus-assert
Jul 14, 2022
Merged

Remove an assert from impHWIntrinsic#72084
EgorBo merged 3 commits into
dotnet:mainfrom
EgorBo:fix-bogus-assert

Conversation

@EgorBo

Copy link
Copy Markdown
Member

Fixes#72081

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 13, 2022
@ghostghost assigned EgorBoJul 13, 2022
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #72081

Author:EgorBo
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@EgorBo

EgorBo commented Jul 13, 2022

Copy link
Copy Markdown
MemberAuthor

hm.. the test I added crashes mono llvmaot https://dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_apis/build/builds/1877495/logs/1885 cc @fanyang-mono@vargaz any quick fix I can apply or just add it to the ignore list and file an issue?

 * Assertion: should not be reached at /__w/1/s/src/mono/mono/mini/simd-intrinsics.c:627

@vargaz

Copy link
Copy Markdown
Contributor

UInt128 is probably not allowed as a simd type by mono, so I'd suggest ignoring the test for now.

@EgorBo

EgorBo commented Jul 13, 2022

Copy link
Copy Markdown
MemberAuthor

UInt128 is probably not allowed as a simd type by mono, so I'd suggest ignoring the test for now.

From my understanding it's also not supported on CLR, it's expected to throw a not-supported-exception rather than an assert

@EgorBo

Copy link
Copy Markdown
MemberAuthor

the fix I've just applied seems to work - it also guards from using Vector128<char> which is a quite popular mistake users make 🙂

@tannergooding

Copy link
Copy Markdown
Member

From my understanding it's also not supported on CLR, it's expected to throw a not-supported-exception rather than an assert

Right, Vector128<UInt128> might be declared by a user and so the runtime should robustly handle it. In this case it should at least fallback to the software path which will then throw NotSupportedException since its an unsupported T.

@vargaz

Copy link
Copy Markdown
Contributor

The mono change looks ok.

@fanyang-mono

Copy link
Copy Markdown
Member

Same check should probably be added to SN_CreateScalar and SN_CreateScalarUnsafe as well.

@fanyang-mono

Copy link
Copy Markdown
Member

Same check should probably be added to SN_CreateScalar and SN_CreateScalarUnsafe as well.

I just checked that CoreCLR doesn't those exceptions for unsupported types when calling the above two methods, then mono doesn't need to do so either. Your fix for mono looks good to me.

@EgorBo

Copy link
Copy Markdown
MemberAuthor

There were still asserts, turns out I didn't test properly and didn't realize that Vector128.Create has generic and non-generic overloads, now it works OK, I validated Mono-LLVM codegen for:

[MethodImpl(MethodImplOptions.NoInlining)]staticVector128<int>Foo1()=>Vector128.Create(42);// non-generic[MethodImpl(MethodImplOptions.NoInlining)]staticVector128<int>Foo2()=>Vector128.Create<int>(42);// generic[MethodImpl(MethodImplOptions.NoInlining)]staticVector128<char>Foo3()=>Vector128.Create<char>('4');[MethodImpl(MethodImplOptions.NoInlining)]staticVector128<Int128>Foo4()=>Vector128.Create<Int128>(42);

Foo1 and Foo2 emit correct broadcast instructions while Foo3 and Foo4 emit throw new NotSupportedException

@EgorBo

Copy link
Copy Markdown
MemberAuthor

CI failure is unrelated: #72182

@EgorBo
EgorBo merged commit 0cc170c into dotnet:mainJul 14, 2022
@EgorBo
EgorBo deleted the fix-bogus-assert branch July 14, 2022 23:29
@ghostghost locked as resolved and limited conversation to collaborators Aug 14, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

Assertion failed 'sizeBytes != 0'

5 participants

@EgorBo@vargaz@tannergooding@fanyang-mono@jakobbotsch