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.

Sign up for GitHub

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

help compiler folding for divideFloor/moduloFloor #4512

Merged
merged 1 commit into from
Mar 12, 2024
Merged

help compiler folding for divideFloor/moduloFloor #4512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ cc_library(
":config_h",
"@boost//:format",
"@boost//:multiprecision",
"@com_google_absl//absl/numeric:bits",
"@com_google_googletest//:gtest",
],
)
Expand Down
1 change: 1 addition & 0 deletions lib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,4 @@ add_library(p4ctoolkit STATIC ${LIBP4CTOOLKIT_SRCS})

# Disable libcall (realloc, malloc) optimizations which may cause infinite loops.
set_target_properties(p4ctoolkit PROPERTIES COMPILE_FLAGS -fno-builtin)
target_link_libraries(p4ctoolkit absl::bits)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asl Another comment I lost due to force-pushing:

It should not be private as bits are used in the header. If you'd make it PUBLIC, then you'd not need to modify every user (like gtest below)

I copied and pasted from one of the other places where we were depending upon an Abseil library (before changing it to bits). That usage had the PRIVATE, which is why I copied it here originally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, right. In few other places we only depend inside .cpp, so PRIVATE is a way to reduce visibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this seems to work, is there a "more correct" solution here?

My concern is that I'm trying to fix the problem of the .h file not being present at compile time. Doesn't target_link_libraries only impact linking: couldn't cmake still try the cpp compilation before the header files are available?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't target_link_libraries only impact linking:

Despite the name – it impacts compile time as well. As it pulls the interface include paths and other properties. So, this is the correct way of doing things. See https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#target-usage-requirements:

Generally, a dependency should be specified in a use of target_link_libraries() with the PRIVATE keyword if it is used by only the implementation of a library, and not in the header files. If a dependency is additionally used in the header files of a library (e.g. for class inheritance), then it should be specified as a PUBLIC dependency. A dependency which is not used by the implementation of a library, but only by its headers should be specified as an INTERFACE dependency. The target_link_libraries() command may be invoked with multiple uses of each keyword:

22 changes: 20 additions & 2 deletions lib/bitrange.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ limitations under the License.
#include <optional>
#include <utility>

#include "absl/numeric/bits.h"
#include "bitvec.h"
#include "exceptions.h"
#include "hash.h"
Expand Down Expand Up @@ -71,7 +72,16 @@ namespace Detail {
* rounds towards zero. For example, `-7 / 8 == 0`, but
* `divideFloor(-7, 8) == -1`.
*/
inline int divideFloor(int dividend, int divisor) {
constexpr inline int divideFloor(int dividend, int divisor) {
#if defined(__GNUC__) || defined(__clang__)
// Code to enable compiler folding when the divisor is a power-of-two compile-time constant
// In this case, compiler should fold to a right-shift
// FIXME: Replace absl with std after moving to C++20
unsigned u_divisor = static_cast<unsigned>(divisor);
if (__builtin_constant_p(u_divisor) && absl::has_single_bit(u_divisor))
return dividend >> (absl::bit_width(u_divisor) - 1);
#endif // defined(__GNUC__) || defined(__clang__)

const int quotient = dividend / divisor;
const int remainder = dividend % divisor;
if ((remainder != 0) && ((remainder < 0) != (divisor < 0))) return quotient - 1;
Expand Down Expand Up @@ -102,7 +112,15 @@ constexpr int modulo(int dividend, int divisor) {
* dividends than for negative dividends.
* To make this concrete, `-7 % 8 == -7`, but `moduloFloor(-7, 8) == 1`.
*/
inline int moduloFloor(const int dividend, const int divisor) {
constexpr inline int moduloFloor(const int dividend, const int divisor) {
#if defined(__GNUC__) || defined(__clang__)
// Code to enable compiler folding when the divisor is a power-of-two compile-time constant
// In this case, compiler should fold to a bitwise-and
// FIXME: Replace absl with std after moving to C++20
if (__builtin_constant_p(divisor) && absl::has_single_bit(static_cast<unsigned>(divisor)))
return dividend & (divisor - 1);
#endif // defined(__GNUC__) || defined(__clang__)

const int remainder = modulo(dividend, divisor);
if (remainder == 0 || dividend >= 0) return remainder;
return divisor - remainder;
Expand Down
Loading