Skip to content
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

Some updates #1

Open
jan-wassenberg opened this issue Jul 16, 2021 · 4 comments
Open

Some updates #1

jan-wassenberg opened this issue Jul 16, 2021 · 4 comments

Comments

@jan-wassenberg
Copy link
Collaborator

Hi, thanks for farm_sve! I've hooked it up to Highway's tests and noticed some issues:

  • LD1RQ: is the predicate really for individual bits? svptrue doesn't set that many. It would make more sense if the predicate were for the elements, no? (Unfortunately the ARM docs do not clearly specify this.)
  • svunpklo_s/u behave like hi (also adding Size/2)
  • svunpkhi_b depends on the size of the type; should instead have one bit/byte per byte
  • svsplice: op2 index should start from zero
  • svbic: need ~ instead of !
  • svunpklo_u64: loop bound should be op.Size / 2
  • core_qsub: adds instead of sub; did not handle 0 - 1 correctly
  • svmaxv: use lowest instead of min
  • core_abs: add if (v1 == std::numeric_limits::min) return v1;
  • svmul_u/s16: cast to u/i32, u/s32 cast to u/i64
  • core_ceil_type_n: just set rounding mode, return nearbyint
  • svtbl: not bounds checked, overrun if > 128
  • svext: op2 index should start from 0
  • core_svrevw: std::swap(data.bt[idx], data.bt[sizeof(Type)/4 - idx - 1]);

These are implemented in patch.txt, which is applied to the clang-format output of wget https://gitlab.inria.fr/bramas/farm-sve/-/raw/master/farm_sve.h?inline=false.

Note that svunpkhi_b is a hack: we need it to expand a 16-bit mask to 32-bit, and this is hardcoded in the implementation. To exactly match SVE, I think we'd need to have one predicate bit per byte in the type, then we could have a type-agnostic unpklo/hi_b.

Please let me know if you disagree with any of the other changes.

@jan-wassenberg
Copy link
Collaborator Author

One more which I omitted upon seeing the latest commit message: although svundef2* are implemented, svundef_u8 etc. for single vectors are not, though they are provided by llvm's arm_sve.h (example).

@berenger-eu
Copy link
Owner

berenger-eu commented Jul 16, 2021

@jan-wassenberg I am not use to github but finally I think it would be more efficient if you could push the code directly (with a PR?), and feel free to add you to the readme as author if you want.
I agree with your list of changes.
Thanks a lot!

@jan-wassenberg
Copy link
Collaborator Author

@berenger-eu thanks for your trust. I'm also (still) not a Git expert but should manage.

Would it be OK if I sent an PR which is the result of running clang-format, then the patch?

jan-wassenberg added a commit to jan-wassenberg/farm-sve that referenced this issue Jul 21, 2021
berenger-eu added a commit that referenced this issue Jul 21, 2021
Output of clang-format -style=Google -i farm_sve.h; refs #1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants
@berenger-eu @jan-wassenberg and others