- User Since
- Jan 7 2022, 12:01 PM (90 w, 8 h)
Aug 11 2023
Superseded by D156478.
Apr 30 2023
Thank you Craig.
Can someone with commit access please land the patch for me? Thanks!
Apr 28 2023
Sep 29 2022
The glibc Bugzilla ticket probably makes the best reading so far: https://sourceware.org/bugzilla/show_bug.cgi?id=20024
Here is the discussion that lead up to the glibc / libmvec commit linked above: https://marc.info/?t=146472287500003
The failing LLVM.Transforms/LoopVectorize/AArch64::scalable-call.ll test makes me wonder what the exact semantics of the vector-function-abi-variant call attribute is in the presence of pointer arguments.
Reset parent to reflect reviewer comment there (removing duplicate call && test).
Re-uploading once again. arc diff --preview creates a preview that looks correct, but just arc diff is somehow mixing in the other patch I currently have open on top of this one.
Re-uploading patch after follow-on patch got mixed in
Sep 28 2022
Add tests using #pragma clang loop vectorize(...).
- Remove duplicate checks
To finally get this out of the way without having the necessary analysis available, I am retreating a bit and just allow forced auto-vectorization via #pragma clang loop vectorize(assume_safety).
Sep 27 2022
It's been a while since I last looked at this. However there don't seem to be many users of -fveclib=libmvec in general as demonstrated by the fact that it has been broken since before I've submitted this patch.
Before returning to allowing sincos() vectorization, I've thus submitted a separate patch to reenable vectorization of all other functions declared in VecFuncs.def already: D134732.
May 17 2022
Apr 21 2022
Apr 20 2022
Apr 14 2022
Mar 14 2022
- at newlines at end of file where missing
- increase nest count
- add nounroll sincos() tests
- make sincos test fail reliably even in single precision
- protect against wrongly computed reference results by printing a few values
Mar 6 2022
- compute reference result in long double
- run tests in both single and double precision
- run libmvec tests for each supported ISA
- don't depend on immintrin.h
Mar 4 2022
- handle 32 bit pointer size
- check for AVX2, not SSE2
Add check that libmvec is available for linking and at runtime
and rename Libmvec/ directory to Veclib/ to indicate the test is
not specific to libmvec and might be enabled for other vector
libraries as well.
Are you talking about automatically choosing a vector library (-fveclib)?
Even if we run the test unconditionally, some logic will still be required to select the right flags to enable auto-vectorization.
I am not sure if it should be Libmvec or X86 specific though. There are other vector libraries on different platforms that provide vector versions of sincos. I don't think we should exclude those from this testing here.
my previous update seems to have eaten the original changes.
Let's see if I can get this right.
I have just submitted the test for the testsuite. Once that is merged, this change should also be fine to go in.
fix mixed up comments
This was supposed to update https://reviews.llvm.org/D120977 instead of creating a new submission.
Jan 12 2022
And a variant of the code above shows that even the transformation to the vvv variant isn't safe in all cases.
I am looking into adding variations of the code above into the test-suite ahead of enabling the vectorizing transformation, to be sure the transformation is not applied when unsafe, and that the behavior of the underlying vector library matches my interpretation of the VectorAPI.
I don't see any existing tests around the transformations already performed.
Also, I see lots of regression tests to prevent potential performance regressions, ensuring the vectorizing transformation is not missed. But I don't see any tests currently to guard against correctness issues, ensuring the transformation is not applied in unsafe cases.
Jan 10 2022
I have beefed up my testcase to demonstrate why I had to choose the _ZGVdN4vvv_sincos() variant for correctness, even though _ZGVdN4vl8l8_sincos() would be desirable from a performance perspective:
We have no control over what pointers the user is passing in in different loop iterations.
Jan 9 2022
I do not have commit access anyway, so can someone please commit once review is sufficient.
A potential issue is that working sincos() support in libmvec is "only" a bit over 5 years old.
E.g. a user on Centos 7 compiling code with a vectorizable call to sincos() and compiling with -fveclib=libmvec and -O2 or higher will now run into an undefined reference to the vectorized sincos() function and will either have to deactivate auto-vectorization or update their libmvec.
I haven't figured out how to link to https://bugs.llvm.org/show_bug.cgi?id=51530 so that it would automatically get closed on merging. I suppose this isn't possible anymore with bugzilla being frozen?
In line with existing behavior, I have not added vector function definitions for AVX-512.
Of the two reproducers linked in https://bugs.llvm.org/show_bug.cgi?id=51530, the sincos_simd.cpp now auto-vectorizes when compiling with clang++ -fveclib=libmvec -O2 -march=core-avx2 sincos_simd.cpp, even when the #pragma omp simd annotation is left out in the source.
Adding reviewers who commented on D88154.
Jan 8 2022
This is my first activity on reviews.llvm.org, so I might just be lacking suitable permissions - I still don't see any way to close.
Jan 7 2022
I tried to mark this patch as a duplicate of the merged https://reviews.llvm.org/D32888 and ideally close it as such, but could not figure out how.