CGAL_ASSUME should use __builtin_assume() or [[assume]] (#7334)#9355
CGAL_ASSUME should use __builtin_assume() or [[assume]] (#7334)#9355RajdeepKushwaha5 wants to merge 1 commit intoCGAL:mainfrom
CGAL_ASSUME should use __builtin_assume() or [[assume]] (#7334)#9355Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates CGAL’s portability/config layer to make CGAL_ASSUME(EX) a true “assumption” (i.e., zero-cost and without runtime evaluation of EX) on more compilers, improving optimization opportunities and avoiding unintended side-effects in optimized builds.
Changes:
- Add a
__builtin_assume(EX)implementation path (via__has_builtin(__builtin_assume)) ahead of the__builtin_unreachablefallback. - Clarify comments to document which
CGAL_ASSUMEvariants do vs. do not evaluateEXat runtime.
Comments suppressed due to low confidence (1)
Installation/include/CGAL/config.h:370
- The
__has_builtin(__builtin_assume)branch unconditionally definesCGAL_UNREACHABLE()as__builtin_unreachable(), but the preprocessor condition does not verify that__builtin_unreachableis available. For portability/feature-detection correctness, either include__has_builtin(__builtin_unreachable)in the condition, or provide a separate fallback definition forCGAL_UNREACHABLE()when__builtin_unreachableis not supported.
#elif __has_builtin(__builtin_assume)
// Clang (and compilers supporting __builtin_assume): the expression is NOT
// evaluated, making it a true zero-cost assumption hint.
# define CGAL_ASSUME(EX) __builtin_assume(EX)
# define CGAL_UNREACHABLE() __builtin_unreachable()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note that |
…in_unreachable() on GCC Add __builtin_assume(EX) path for Clang/ICC (zero-cost, no evaluation). Exclude GCC from C++23 [[assume(EX)]] path because __builtin_unreachable() enables stronger optimizations on GCC (where [[assume]] is often ignored). Priority order: 1. [[assume(EX)]] - C++23, but NOT on GCC 2. __builtin_assume(EX) - Clang/ICC, no evaluation 3. __builtin_unreachable - GCC (always), stronger optimizations 4. __assume(EX) - MSVC, no evaluation Fixes CGAL#7334
99e7a1d to
e7ee58f
Compare
Thanks for the detailed feedback @mglisse — that's a really important nuance about GCC's I've updated the PR to address this. The changes:
The guard is: Updated priority order:
This way GCC keeps the stronger |
Problem:
The
CGAL_ASSUME(EX)macro (inInstallation/include/CGAL/config.h) used the following implementation for GCC/Clang:This evaluates
EXat runtime as part of theif-condition, even in optimised builds. That has non-zero overhead and potential side-effects, even though the entire purpose of the macro is to give the compiler a zero-cost optimisation hint.Clang (and Intel ICC) support
__builtin_assume(expr)which is a true assumption:the expression is never evaluated and the compiler uses it purely as a hint.
Fix: Added a new
#elif __has_builtin(__builtin_assume)branch before the__builtin_unreachablefallback branch. Priority order is now:[[assume(EX)]]— no expression evaluation, portable__builtin_assume(EX)— Clang/ICC, no evaluation (new)if(!(EX)){ __builtin_unreachable(); }— GCC fallback, evaluates EX__assume(EX)— MSVC, no evaluationAlso improved comments to clearly document which forms evaluate the expression and which do not.
Files changed:
Installation/include/CGAL/config.h(+9/-2 lines)