Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make native code portable and add GitHub workflow for building #949
Make native code portable and add GitHub workflow for building #949
Changes from 31 commits
b263fc6
de10f7e
0c62fa6
aae5ff7
b06590d
03744cb
648e2f5
cba2b1a
6f70a5e
90fa8b1
c815ca0
36b1ef2
44e3f17
6fe8d0c
572225e
7a8676e
771f6db
8dd8d63
8b1ceb7
e11867b
57625db
ada2e9a
e0093e9
23bdf05
87414c3
0ee8f7f
1f35064
816eee0
0528324
8152e21
9aad25a
bcc6780
2951e2c
9867392
b773dfb
45ad394
e2e4874
59a1000
41ddd25
b5b6151
ca5f14a
cd8343e
2456cf3
b460125
7a605e1
28188a5
86b2bd6
01c3f59
e4344b0
e2e8f87
2ba8be3
7ed6a01
eba19a8
3288a0f
fdddb11
b7503c9
fb642a5
2730dd9
2e3a1d8
590a27a
927f716
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also user facing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code fails to compile on a lower target. For my understanding, why would you want to change this without chaning source code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you using AVX2 intrinsic? Otherwise this is also a user decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Celeron does not support AVX2 for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also override user settings of
/arch:AVX512
since MSVC takes the last flagThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The source code uses AVX2 intrinsics. "user" here is developer, i.e. you need to change source code to make use of AVX512.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is incorrect. MSVC can compile AVX2 intrinsic without the
/ARCH:AVX2
since most intrinsics are visible fromimmintrin.h
. The /ARCH flags just allows the compiler to use these intrinsics itself while optimizing the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry if I was unclear. I just meant that since the AVX2 intrinsics is part of the source code, does it make sense to expose this to the end user compiling from source?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, I don't mind removing this or making it overridable. I just want to understand how to design the replacement :) For example, should we add a SIMD option that would allow the end user to switch between scalar, AVX2, AVX512 and (on arm ) Neon?