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

Add JIT-helper of ArgTypeNotSupportedException#15028

Closed
fiigii wants to merge 1 commit into
dotnet:masterfrom
fiigii:typeexp
Closed

Add JIT-helper of ArgTypeNotSupportedException#15028
fiigii wants to merge 1 commit into
dotnet:masterfrom
fiigii:typeexp

Conversation

@fiigii
Copy link
Copy Markdown

@fiigii fiigii commented Nov 14, 2017

Intel hardware intrinsics need to throw exceptions with System.NotSupportedException: Specified type is not supported on Vector128/256<T> that is instantiated by non-numeric types.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Nov 14, 2017

@CarolEidt @jkotas Would you like to port this change to desktop CLR at first? I need this jit-helper in my Vector128/256<T> work.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Nov 15, 2017

You will need to change JIT interface GUID at the top of corinfo..h for this change.

cc @dotnet/jit-contrib

Comment thread src/inc/corinfo.h Outdated
CORINFO_HELP_THROW_ARGUMENTEXCEPTION, // throw ArgumentException
CORINFO_HELP_THROW_ARGUMENTOUTOFRANGEEXCEPTION, // throw ArgumentOutOfRangeException
CORINFO_HELP_THROW_PLATFORM_NOT_SUPPORTED, // throw PlatformNotSupportedException
CORINFO_HELP_THROW_ARGTYPE_NOT_SUPPORTED, // throw NotSupportedException
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A nit - comment alignment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thank you, will fix.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Nov 15, 2017

You will need to change JIT interface GUID at the top of corinfo..h for this change.

@jkotas I see. Shall I update GUID now? Or wait for the update from desktop CLR change?

@briansull
Copy link
Copy Markdown

Go ahead and make the GUID change as part of your checkin.

We will need to check in the src/inc/corinfo.h changes on the Desktop as a separate step.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Nov 15, 2017

@briansull Got it, thank you.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Nov 16, 2017

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test
@dotnet-bot test Ubuntu16.04 armlb Cross Debug Innerloop Build

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Nov 16, 2017

@jkotas The test failures look unrelated to this change?

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Nov 16, 2017

Actually, I do not think you need a helper for this. Why can't the jit fallback to the non-inlined implementation to throw the exception?

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Nov 16, 2017

Why can't the jit fallback to the non-inlined implementation to throw the exception?

@jkotas Do you mean something like this?

public static Vector128<T> GetLowerHalf<T>(Vector256<T> value) where T : struct 
{
    if (typeof(T) != typeof(Byte) && typeof(T) != typeof(UByte) && ... )
    {
        throw new NotSupportedException(SR.Arg_TypeNotSupported);
    }
    return GetLowerHalf<T>(value);
}

This is a good point. However, our current contract is "named intrinsics that directly called by itself must be expanded". If we add the exception code in the managed implementation, that would break this contract. Then other named intrinsics cannot have the fallback with any recursive algorithm in the future. I am afraid that might be a too strict limitation.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Nov 16, 2017

Right, something like this.

I would think that gtIsRecursiveCall will still return true for the above case and the inner call will be force expanded. If there is something that I am missing, we should fix it.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Nov 16, 2017

I would think that gtIsRecursiveCall will still return true for the above case and the inner call will be force expanded.

I meant maybe other recursive intrinsics (in the future) that we still want them optionally expanded and only "directly recursive" intrinsics are forced expanded.
But, at this time, your solution is reasonable to me. If you think we do not need to consider the above situation, I will close this PR.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Nov 16, 2017

Right, I do not think we need to worry about it - until we have a real situation like that.

@jkotas jkotas closed this Nov 16, 2017
@fiigii fiigii deleted the typeexp branch November 17, 2017 01:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants