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

Fix GitHub #20389 #20391

Merged
merged 1 commit into from
Oct 15, 2018
Merged

Fix GitHub #20389 #20391

merged 1 commit into from
Oct 15, 2018

Conversation

fiigii
Copy link

@fiigii fiigii commented Oct 12, 2018

@danmoseley
Copy link
Member

Why does it need to check for Avx2 if its only using Avx?

@fiigii
Copy link
Author

fiigii commented Oct 13, 2018

Integer SIMD addition instructions (please see Vector256Add<T>) are provided by AVX2, and AVX just has floating point instructions.

@danmoseley
Copy link
Member

So if I understand right, public static Vector256<T> SetAllVector256<T>(T value) where T : struct (which is not on the Avx2 class) requires Avx2 when T is an integer type? If so, it seems to me this is likely to be a common mistake, which presumably will only show up when someone tests on a machine that does not support AVX2?

@tannergooding

@tannergooding
Copy link
Member

The issue here is with the Vector256Add call (which calls Avx2.Add:

return Avx.StaticCast<byte, T>(Avx2.Add(Avx.StaticCast<T, byte>(left), Avx.StaticCast<T, byte>(right)));
).

SetAllVector256 is a helper method and is being moved to the Vector256<T> type as part of #20147 (as soon as I push the changes). It will be available always.

@danmoseley
Copy link
Member

Oops, yes I quoted the wrong API. OK, after that change I should never need if (Avx2.IsSupported) unless I am using the Avx2 class?

@tannergooding
Copy link
Member

Correct.

@fiigii
Copy link
Author

fiigii commented Oct 15, 2018

@tannergooding Thanks for the reply 😄
@danmosemsft Can we merge this PR?

@danmoseley
Copy link
Member

@tannergooding can sign off

@tannergooding tannergooding merged commit 7c04af4 into dotnet:master Oct 15, 2018
@fiigii fiigii deleted the fix20389 branch October 24, 2018 22:25
A-And pushed a commit to A-And/coreclr that referenced this pull request Nov 20, 2018
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.

4 participants