Skip to content

Conversation

@AndyAyersMS
Copy link
Member

Though this method is only ever callable by code that's already done the right
IsSupported check, add a redundant check to the method itself, so that when
crossgenning SPC on non-xarch platforms we won't try and compile the xarch
specific parts of this method. This should save some time and a bit of file
size for non-xarch SPCs.

See notes on #38894 for context.

Though this method is only ever callable by code that's already done the right
IsSupported check, add a redundant check to the method itself, so that when
crossgenning SPC on non-xarch platforms we won't try and compile the xarch
specific parts of this method.  This should save some time and a bit of file
size for non-xarch SPCs.

See notes on dotnet#38894 for context.
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 14, 2020
@AndyAyersMS
Copy link
Member Author

@benaadams
Copy link
Member

Will this be an issue for other methods that are only called by Sse2.IsSupported guarded points but don't have the check themselves? e.g.

private static unsafe nuint NarrowUtf16ToAscii_Sse2(char* pUtf16Buffer, byte* pAsciiBuffer, nuint elementCount)
{
// This method contains logic optimized for both SSE2 and SSE41. Much of the logic in this method
// will be elided by JIT once we determine which specific ISAs we support.
// JIT turns the below into constants

@davidwrighton
Copy link
Member

@benaadams Yes, it is a minor issue. It may cause a small amount of size inflation for all the non-X86 platforms. Fortunately, the behavior should not be incorrect either way, as long as the missed check is for Sse2 or Sse, as all X86 that we support supports Sse2. (If there are similar missing checks for Avx or something though, that's a different story, and a correctness bug.) To make it clear that in general its required to have these checks (when writing code in System.Private.CoreLib) in each individual method that uses the intrinsics, I think its not a bad idea to put them in for cases like this.

@AndyAyersMS AndyAyersMS merged commit cc6d54c into dotnet:master Jul 17, 2020
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
Though this method is only ever callable by code that's already done the right
IsSupported check, add a redundant check to the method itself, so that when
crossgenning SPC on non-xarch platforms we won't try and compile the xarch
specific parts of this method.  This should save some time and a bit of file
size for non-xarch SPCs.

See notes on dotnet#38894 for context.
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR 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.

5 participants