Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

CORINFO_HELP_THROW_TYPE_NOT_SUPPORTED#16102

Merged
tannergooding merged 1 commit into
dotnet:masterfrom
sdmaclea:PR-CORINFO_HELP_THROW_TYPE_NOT_SUPPORTED
Jan 31, 2018
Merged

CORINFO_HELP_THROW_TYPE_NOT_SUPPORTED#16102
tannergooding merged 1 commit into
dotnet:masterfrom
sdmaclea:PR-CORINFO_HELP_THROW_TYPE_NOT_SUPPORTED

Conversation

@sdmaclea
Copy link
Copy Markdown

@sdmaclea sdmaclea commented Jan 30, 2018

@jkotas @CarolEidt @tannergooding PTAL
@dotnet/jit-contrib FYI

@sdmaclea
Copy link
Copy Markdown
Author

sdmaclea commented Jan 30, 2018

JIT of HW intrinsics will use this to throw from generics instantiated with unsupported types

i.e. Vector64<bool> or Vector128<Vector64<byte>>

@sdmaclea
Copy link
Copy Markdown
Author

sdmaclea commented Jan 30, 2018

Tested on Arm64 using #16008 testThrowsTypeNotSupported

@sdmaclea sdmaclea mentioned this pull request Jan 30, 2018
Merged
fiigii
fiigii reviewed Jan 30, 2018
View reviewed changes
Comment thread src/vm/jithelpers.cpp Outdated

HELPER_METHOD_FRAME_BEGIN_ATTRIB_NOPOLL(Frame::FRAME_ATTR_EXCEPTION); // Set up a frame

COMPlusThrow(kNotSupportedException, W("NotSupported_Type"));
Copy link
Copy Markdown

@fiigii fiigii Jan 30, 2018

Choose a reason for hiding this comment

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

This behavior is slightly different from S.N.Vector<T> that throws NSE with message "Specified type is not supported".

#15028

Copy link
Copy Markdown
Author

@sdmaclea sdmaclea Jan 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is the message the VM typically uses for this exception. I see no reason it has to match S.N.Vector<T>.

Copy link
Copy Markdown
Member

@tannergooding tannergooding Jan 30, 2018

Choose a reason for hiding this comment

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

The only difference between the two is the word "Specified", which might make it more clear that the type itself(Vector128<T>) is supported, its only the type of the generic which is not.

I don't have a strong preference either way, however.

Copy link
Copy Markdown

@fiigii fiigii Jan 30, 2018

Choose a reason for hiding this comment

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

I see no reason it has to match S.N.Vector

The only reason is that we may implement S.N.Vector<T> via managed code in the future.

Copy link
Copy Markdown
Member

@tannergooding tannergooding Jan 30, 2018

Choose a reason for hiding this comment

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

The only reason is that we may implement S.N.Vector via managed code in the future.

Even then, we could just manually validate ahead of time (which we probably would need to do anyways, since we have "exploded" rather than purely generic APIs, due to the ISA splits).

Copy link
Copy Markdown
Author

@sdmaclea sdmaclea Jan 30, 2018

Choose a reason for hiding this comment

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

The other reason would be to match

ThrowHelper.ThrowNotSupportedExceptionIfNonNumericType<T>()

which is currently using SR.Arg_TypeNotSupported

Copy link
Copy Markdown

@CarolEidt CarolEidt Jan 30, 2018

Choose a reason for hiding this comment

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

It would seem to make sense to me to match ThrowHelper.ThrowNotSupportedExceptionIfNonNumericType<T>()

Copy link
Copy Markdown
Author

@sdmaclea sdmaclea Jan 30, 2018

Choose a reason for hiding this comment

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

Done

@sdmaclea sdmaclea force-pushed the PR-CORINFO_HELP_THROW_TYPE_NOT_SUPPORTED branch from d4a3ca2 to 0623a4f Compare January 30, 2018 22:27
CarolEidt
CarolEidt approved these changes Jan 30, 2018
View reviewed changes
Copy link
Copy Markdown

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@tannergooding
Copy link
Copy Markdown
Member

tannergooding commented Jan 30, 2018 •
edited
Loading

@fiigii, it would probably be good to update the x86 code to use this as well.

@fiigii
Copy link
Copy Markdown

fiigii commented Jan 30, 2018

it would probably be good to update the x86 code to use this as well.

Yes, I will.

@sdmaclea sdmaclea mentioned this pull request Jan 30, 2018
Merged
@sdmaclea
Copy link
Copy Markdown
Author

sdmaclea commented Jan 31, 2018

Arm64 intermittent fail
15:51:37 Pass: 1218 Fail: 0 Complete: 62% Start:15:51:35 Test:[_il_reljumper4.cmd_7724] JIT\Methodical\VT\callconv\_il_reljumper4\_il_reljumper4.cmd 15:51:37 Pass: 1218 Fail: 1 Complete: 62% Start:15:51:35 Test:[_il_relthread-race.cmd_7595] JIT\Methodical\tailcall\Desktop\_il_relthread-race\_il_relthread-race.cmd

test Windows_NT arm64 Cross Checked Innerloop Build and Test

@sdmaclea
Copy link
Copy Markdown
Author

sdmaclea commented Jan 31, 2018

Please merge

@tannergooding tannergooding merged commit c36ea9f into dotnet:master Jan 31, 2018
@sdmaclea sdmaclea deleted the PR-CORINFO_HELP_THROW_TYPE_NOT_SUPPORTED branch May 24, 2018 19:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@fiigii fiigii fiigii left review comments

@CarolEidt CarolEidt CarolEidt approved these changes

@tannergooding tannergooding tannergooding approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-CodeGen

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sdmaclea @tannergooding @fiigii @CarolEidt @jkotas