Enable partially implemented Intel HW intrinsics ISAs - CoreCLR part of changes

[4creators]

No description provided.

[4creators]

CoreFX part of changes is in PR dotnet/corefx#28377

[CarolEidt]

Do we not want to make this change in master? There will be another round of changes ported to the 2.1 branch, so I think this change should be made there, unless we explicitly want to retain the not-supported behavior. @RussKeldorph can confirm.

[4creators]

There will be another round of changes ported to the 2.1 branch, so I think this change should be made there, unless we explicitly want to retain the not-supported behavior.

@CarolEidt I am fine with either way of implementing this change. Should I move this PR to master or wait for @RussKeldorph ?

[CarolEidt]

Let's wait and see, as I think it should probably go into master. I believe that with the "versioned with the target framework" model we will be keeping the corefx apis in sync with the JIT implementation.

[RussKeldorph]

If the corresponding change is going into corefx master, the should go to coreclr master. Did I understand the question?

[fiigii]

The release/2.1 branch seems not aware of the current intrinsic implementations, I am not sure if this is the good time to make this change...

If we only move Avx, Avx2, and Sse42 to the "fully-implemented" state (no change for mscorlib APIs), I guess the PR to the master should be fine (easy to revert).

[fiigii]

I do not think we need the new comment. JIT implementation is not related to corefx surface and the S.R.Intrinsic.Experimental package is a temporary solution.

[4creators]

S.R.Intrinsic.Experimental package is a temporary solution

Agree, however, function logic has changed and old description is wrong. Perhaps I will add TODO-XARCH-Cq to describe it and remember to update function and comment in future. I would expect that once we have all ISAs implemented we will not need this check and function anymore.

[fiigii]

we will not need this check and function anymore.

Not really, I think that we always need this logic for future work (e.g., AVX-512) even though the current ISA support gets completed.

[4creators]

I think that we always need this logic for future work (e.g., AVX-512)

Yes, you are right, but only under the condition that we will follow implementation pattern where APIs are exposed first and implemented later.

[4creators]

If the corresponding change is going into corefx master, the should go to coreclr master.

@CarolEidt @RussKeldorph

We can make this coreclr change to master, and than we would enable all implemented APIs for both master and release/2.1, and we can remove all not implemented APIs from release/2.1 branch in corefx or booth (by making change to corefx master) than:

1. This change in coreclr master will propagate into coreclr release/2.1 branch and corefx master and release/2.1 branches since there will be a merge from master into release/2.1 soon.
2. We can keep corefx release/2.1 and master branches synchronized with ongoing implementation of APIs in RyuJIT.

As far as I understand the problem we should decide on is

1. If we want this PR in master coreclr

2. If we want related PR in both corefx master and in release/2.1 branches or only in release/2.1 branch -

3. Applying change only to corefx release/2.1 while having it committed to coreclr master will create unwanted situation where ISA.IsSupported check for support of intrinsics will be false positive on master for incomplete APIs unless we remove this APIs from corefx master.

The question is:

• Should we merge this changes into coreclr master and corefx master or into corefx release/2.1 and coreclr release/2.1?

IMO we should merge into master in both repos and for tests build make dependency directly on S.P.C. This will allow for clear cut "versioned with the target framework" feature development model and we could get rid of bool Compiler::isFullyImplmentedISAClass(InstructionSet isa) usage. I would propose defering larger refactoring to post 2.1 work and limit current change to this PR.

[4creators]

@CarolEidt @RussKeldorph @eerhardt @fiigii @tannergooding

Pls take a look at the proposed modification of changes.

[eerhardt]

Should we merge this changes into coreclr master and corefx master or into corefx release/2.1 and coreclr release/2.1?

The process is to merge all changes into master first and stabilize there. And then if they are necessary to be ported to a release branch, they are then ported as necessary. So this and the corefx change should all go into master first.

There will be another integration from master into release/2.1, so this change will make it into the .NET Core 2.1 release if it is merged to master in the next week.

[eerhardt]

The code changes look good - but this needs to target master.

[4creators]

Closing as new PR #17184 is targeting master branch