WIP: Needs validation of changes - #9553
Conversation
mglisse
left a comment
There was a problem hiding this comment.
Any word on benchmark results?
| #ifdef CGAL_USE_SSE2_MAX | ||
| CGAL::Max<double> mmax; | ||
|
|
||
| maxx = mmax(maxx, aprx, apsx); | ||
| maxy = mmax(maxy, apry, apsy); | ||
| maxz = mmax(maxz, aprz, apsz); | ||
| #elif defined CGAL_USE_NEON_MAX |
There was a problem hiding this comment.
| #ifdef CGAL_USE_SSE2_MAX | |
| CGAL::Max<double> mmax; | |
| maxx = mmax(maxx, aprx, apsx); | |
| maxy = mmax(maxy, apry, apsy); | |
| maxz = mmax(maxz, aprz, apsz); | |
| #elif defined CGAL_USE_NEON_MAX | |
| #if defined CGAL_USE_SSE2_MAX || defined CGAL_USE_NEON_MAX |
to reduce duplication? It might even make sense to introduce a common macro for both, if they are always used together.
There was a problem hiding this comment.
Thanks for the suggestion @mglisse , I have added a common macro and simplified the simd logic.
| sse2minmax(maxx,maxy,maxz); | ||
| // maxy can contain ANY element | ||
| #endif | ||
| #elif defined CGAL_USE_NEON_MAX | ||
| neon_minmax(maxx,maxy,maxz); |
There was a problem hiding this comment.
If sse2minmax and neon_minmax work the same way, it could help reduce duplication to give them the same name (no opinion on what that name should be).
We could even introduce a helper function (with sort3 in its name?), defined for all platforms, to avoid repeating the whole block of code.
| #include <CGAL/sse2.h> | ||
| #endif | ||
|
|
||
| #ifdef CGAL_USE_NEON_FABS |
There was a problem hiding this comment.
Is this macro automatically defined somewhere, or for now a user has to define it by hand?
There was a problem hiding this comment.
@mglisse
These macros should passed by users while building while using CGAL headers.
| #ifdef CGAL_USE_NEON_FABS | ||
| inline double neon_fabs(double a) | ||
| { | ||
| return vget_lane_f64(vabs_f64(vdup_n_f64(a)), 0);; |
There was a problem hiding this comment.
Is this again specifically for visual studio, while all other compilers do ok with normal code?
There was a problem hiding this comment.
I have been testing this code changes with github windows arm64 runners, so I have made changes with respect to MSVC. MSVC does not support direct NEON simd for double abs. But GNU and Clang-like compilers supports direct NEON double abs.
| #if defined __GNUG__ || defined __clang__ | ||
| // The "+w" constraint pins the value in a NEON/FP register and acts as | ||
| // a compiler barrier without emitting any instruction. | ||
| asm volatile ("" : "+w"(x)); |
There was a problem hiding this comment.
No "+mw" for gcc?
(I haven't played with these things for a while, so I don't know how compilers have evolved since then)
There was a problem hiding this comment.
GCC uses "w" for Floating point register for AARCH64 family. Refer: https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html
| volatile float64x2_t e = x; | ||
| std::memcpy(&x, (void*)&e, 16); |
There was a problem hiding this comment.
I tested this with sample code with compiler explorer. MSVC and GCC produce identical results. The MSVC fallback emits a real memory store/load, matching GCC's behavior for the volatile-copy implementation.
| // MSVC on ARM64: must come first, before CGAL_SAFE_SSE2 and the plain | ||
| // _MSC_VER branch, because _controlfp_s and _MM_SET_ROUNDING_MODE are | ||
| // both x86-only and do not exist on ARM64. |
There was a problem hiding this comment.
Maybe the "plain" _MSC_VER branch should be fixed to test architecture macros at the same time?
Also, why before CGAL_SAFE_SSE2, is that defined for arm as well now?
There was a problem hiding this comment.
The rounding-mode ladder below must test _MSC_VER && _M_ARM64 FIRST,before CGAL_SAFE_SSE2 and before the plain _MSC_VER branch, for two reasons:
- CGAL_SAFE_SSE2 can be defined on MSVC ARM64 if the compiler sets
FLT_EVAL_METHOD==0 (which it does), causing _MM_SET_ROUNDING_MODE
to be selected — but that SSE2 intrinsic does not exist on ARM64. - The plain _MSC_VER branch uses _controlfp_s, which is also x86-only
and absent on ARM64.
The plain _MSC_VER branch is also guarded with !defined(_M_ARM64) below
to make the intent explicit and prevent future regressions.
I was trying to build CGAL benchmarks, but I am hitting lot of errors while building benchmarks. |
|
Hi @mglisse
|
Please commit your benchmark code. |
|
Hi @afabri |
Summary of Changes
Release Management