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

Add pointer overloads for Avx2.BroadcastScalarToVector256#32565

Merged
jkotas merged 1 commit into
dotnet:masterfrom
fiigii:addbc
Oct 6, 2018
Merged

Add pointer overloads for Avx2.BroadcastScalarToVector256#32565
jkotas merged 1 commit into
dotnet:masterfrom
fiigii:addbc

Conversation

@fiigii
Copy link
Copy Markdown
Contributor

@fiigii fiigii commented Oct 1, 2018

@fiigii
Copy link
Copy Markdown
Contributor Author

fiigii commented Oct 3, 2018

@dotnet-bot test this please

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

@fiigii
Copy link
Copy Markdown
Contributor Author

fiigii commented Oct 3, 2018

I have disabled the API baseline checks for the netcoreappaot in the previous PR. But Packaging All Configurations x64 Debug Build fails again here. What should I do for the test? cc @tannergooding @jkotas

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Oct 3, 2018

@fiigii Try to add the <RunApiCompat Condition="'$(TargetGroup)' != 'netcoreappaot'">true</RunApiCompat> condition directly to src/System.Runtime.Intrinsics/src/System.Runtime.Intrinsics.csproj to see whether it makes the errors go away.

If it does work, do the busy work to add the the errors to https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Intrinsics/src/ApiCompatBaseline.netcoreappaot.txt. It will make the errors go away for sure, but I would love to figure out a way how we can avoid doing this busy work for frequently changing assemblies like System.Runtime.Intrinsics.csproj.

cc @morganbr

@morganbr
Copy link
Copy Markdown

morganbr commented Oct 3, 2018

@jkotas, would it work/make sense to make System.Runtime.Intrinsics a partial façade with NotImplementedExceptions? I wonder if that would also be useful for all of the APIs that don't match a particular architecture.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Oct 4, 2018

would it work/make sense to make System.Runtime.Intrinsics a partial façade with NotImplementedExceptions?

As a hack to make the error go away - yes.

As the proper solution for this case - no. The IsSupported properties need to return false, not throw exceptions.

@morganbr
Copy link
Copy Markdown

morganbr commented Oct 4, 2018

I think a partial façade can be implemented with whatever code that's useful (false for IsSupported, NotImplementedException for the intrinsics). @ericstj, does that sound right?

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Oct 4, 2018

I have misunderstood what you meant. I think you meant that the implementation may be coming either from CoreLib or CoreFX depending on the config. Yes, it would be possible in theory.

Having different implementation homes for given type based on config was pain to deal with. We got rid of all places that have done that - the legacy setup for S.P.Interop from ProjectN is the only one left.

@ericstj
Copy link
Copy Markdown
Member

ericstj commented Oct 4, 2018

Cross compiling in corefx with partial impl in source is perfectly reasonable so long as you have a good pivot to cross-compile by.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Oct 4, 2018

Cross compiling in corefx with partial impl in source is perfectly reasonable

We are talking about having different CoreLib public surface based on CPU architecture. We assume that the CoreLib surface is same between different CPU architectures today. If start having different CoreLib surface for different CPU architectures, anything that makes the assumption has to be forked. For example, configurations like https://github.com/dotnet/corefx/blob/master/src/System.Runtime/src/Configurations.props would have to be replicated 4 times for x86/x64/arm/arm64 to account for the differences; etc. I do not think we want to go there.

@morganbr
Copy link
Copy Markdown

morganbr commented Oct 4, 2018

I didn't mean to imply different surface based on CPU architecture, just on TargetGroup.

@morganbr
Copy link
Copy Markdown

morganbr commented Oct 4, 2018

Ok, apparently I did imply that, but I'm happy to drop the CPU part and settle for TargetGroup

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Oct 4, 2018

I'm happy to drop the CPU part and settle for TargetGroup

It would still not address the pain with iterating on System.Runtime.Intrinsics library.

@fiigii
Copy link
Copy Markdown
Contributor Author

fiigii commented Oct 5, 2018

@jkotas I tried the two approaches you suggested, seems only the "busy work" works. Now CI gets green.

@jkotas jkotas merged commit e21fc3d into dotnet:master Oct 6, 2018
@jkotas
Copy link
Copy Markdown
Member

jkotas commented Oct 6, 2018

Thanks

@karelz karelz added this to the 3.0 milestone Oct 8, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
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.

7 participants