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

Add pointer overloads for Avx2.BroadcastScalarToVector128#20055

Merged
tannergooding merged 2 commits into
dotnet:masterfrom
fiigii:addbroadcast
Sep 20, 2018
Merged

Add pointer overloads for Avx2.BroadcastScalarToVector128#20055
tannergooding merged 2 commits into
dotnet:masterfrom
fiigii:addbroadcast

Conversation

@fiigii
Copy link
Copy Markdown

@fiigii fiigii commented Sep 19, 2018

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 19, 2018

@CarolEidt @eerhardt

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 19, 2018

After this PR and #19420 gets merged, I will submit a PR to implement all the remaining AVX2 intrinsic in JIT.

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
Author

fiigii commented Sep 19, 2018

Added comments and disabled tests of BroadcastScalarToVector128<T> that have ambiguities from the new APIs.

/// __m128i _mm_broadcastw_epi16 (__m128i a)
/// VPBROADCASTW xmm, m16
/// The above native signature does not directly correspond to the managed signature.

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.

nit: stray newlines in the middle of the doc comments for most of these.

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.

Oops, will fix.

/// __m128i _mm_broadcastb_epi8 (__m128i a)
/// VPBROADCASTB xmm, m8
/// The above native signature does not directly correspond to the managed signature.

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.

nit: Still one remaining

@tannergooding
Copy link
Copy Markdown
Member

Thanks for the work @fiigii. Feel free to tag me when the tests complete and I'll get this merged.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 20, 2018

@dotnet-bot test this please

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 20, 2018

CI is still stuck... Can we merge this and #19420? I have submitted corefx PR.

@CarolEidt
Copy link
Copy Markdown

Many PRs are stuck on testing, but I'm not comfortable with the idea that we should just merge without completing at least the xarch testing that's in the main CI set.

@tannergooding
Copy link
Copy Markdown
Member

Merging, Two of the pending jobs actually completed. The other was an out of disk space issue, but a similar job (testing additional functionality) succeeded.

For the additional failures, they are also unrelated CI failures and had been previously passing (they were rerun due to a new commit which cleaned up some code comments).

@tannergooding tannergooding merged commit 0f4ecd9 into dotnet:master Sep 20, 2018
@fiigii fiigii deleted the addbroadcast branch September 20, 2018 20:46
@tannergooding
Copy link
Copy Markdown
Member

@CarolEidt, saw your comment after I hit merge. The xarch jobs were all passing, CI just didn't update.

All the jobs had also been passing before the code comment cleanup I asked @fiigii to do, so the minor cleanup that did occur shouldn't have caused any additional regressions in the remaining jobs (which were mostly the perf ones).

@CarolEidt
Copy link
Copy Markdown

@tannergooding - thanks for the clarification, and for the work of tracking down job status.

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.

Avx2.BroadcastScalarToVector128(T*) overloads are missing

4 participants