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

Implement AVX SetHighLow#17313

Merged
CarolEidt merged 1 commit into
dotnet:masterfrom
fiigii:sethilo
Apr 2, 2018
Merged

Implement AVX SetHighLow#17313
CarolEidt merged 1 commit into
dotnet:masterfrom
fiigii:sethilo

Conversation

@fiigii
Copy link
Copy Markdown

@fiigii fiigii commented Mar 29, 2018

I missed Avx.SetHighLow in #17030 ...

@CarolEidt @tannergooding PTAL

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Mar 29, 2018

@CarolEidt can we merge this PR for 2.1 if it looks good to you?

@CarolEidt
Copy link
Copy Markdown

I would prefer not to merge this for 2.1. We are in the final stages of wrapping up as many bugs as possible for 2.1, and I'd prefer not to take the risk of new functionality & tests.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Mar 29, 2018

But we have exposed this API in the corefx package.. sorry my bad.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Mar 30, 2018

There was an oversight that Avx.SetHighLow was missed when I submitted PR #17030. I know that tomorrow is the ZBB date, and we have decided to stop enabling new intrinsic in the runtime.
However, the API of Avx.SetHighLow has been exposed in the CoreFX package. So, I suggest merging this PR for 2.1 to avoid propagating this mistake to CoreFX workflow. Meanwhile, this is just a simple "NoCodegen" helper-intrinsic, which will be converted to Avx.InsertVector128 in the importer, so I believe it won't has any codegen bug to disturb the ZBB plan.

@CarolEidt @AndyAyersMS @tannergooding @eerhardt Do you agree?

@tannergooding
Copy link
Copy Markdown
Member

I think we will need to do one of:

  • Take this PR
  • Take a PR removing the API from CoreFX
  • Do nothing and tell people using the preview to not use SetHighLow

@4creators
Copy link
Copy Markdown

IMO the best option currently is:

  • Take a PR removing the API from CoreFX

as it explictly avoids any risks.

@CarolEidt
Copy link
Copy Markdown

Another possible option is to remove the tests from this PR and merge it. It may be better to have u functionality w/o tests than to risk adding further noise to existing test challenges.

Note that taking a PR to remove the API from CoreFX definitely does not avoid any risks. It's almost as easy to break things when removing something as when adding.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Mar 30, 2018

Another possible option is to remove the tests from this PR and merge it.

Agree, will do.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Mar 30, 2018

Removed the tests.

@4creators
Copy link
Copy Markdown

taking a PR to remove the API from CoreFX definitely does not avoid any risks

I do not agree as API is not used anywhere and in CoreFX we have only facade to S.P.C implementation.

However, I am equally OK with adding implementation of Avx.SetHighLow

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Mar 31, 2018

@CarolEidt Can we merge this PR?

@CarolEidt CarolEidt merged commit 33fed29 into dotnet:master Apr 2, 2018
@fiigii fiigii deleted the sethilo branch April 2, 2018 17: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