Skip to content

Commit

Permalink
address PR feedback from @asl
Browse files Browse the repository at this point in the history
---------

Co-authored-by: Anton Korobeynikov <anton@korobeynikov.info>
  • Loading branch information
grg and asl committed Mar 9, 2024
1 parent 6d444bd commit 67ac50b
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 12 deletions.
1 change: 1 addition & 0 deletions BUILD.bazel
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
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 PRIVATE absl::bits)
22 changes: 11 additions & 11 deletions lib/bitrange.h
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 @@ -65,29 +66,27 @@ class JSONLoader;
namespace BitRange {

namespace Detail {
#if defined(__GNUC__) || defined(__clang__)
/**
* @return boolean indicating if @p value is a power of two.
* @return find the bit position of the first one bit
*
* Will return all-ones if no bit is set
*
* This function is declared constexpr to allow it to be used in tests to optimize code generation.
*/
constexpr bool isPowerOfTwo(int value) { return value && !(value & (value - 1)); }

constexpr int log2ForPowerOfTwo(int value) { return sizeof(value) * 8 - __builtin_clz(value) - 1; }
#endif // defined(__GNUC__) || defined(__clang__)
constexpr unsigned ffs(unsigned value) { return sizeof(value) * 8 - absl::countl_zero(value) - 1; }

/**
* @return the result of dividing @dividend by @divisor, rounded towards
* negative infinity. This is different than normal C++ integer division, which
* 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
if (__builtin_constant_p(divisor) && isPowerOfTwo(divisor))
return dividend >> log2ForPowerOfTwo(divisor);
if (__builtin_constant_p(divisor) && absl::has_single_bit(static_cast<unsigned>(divisor)))
return dividend >> ffs(divisor);
#endif // defined(__GNUC__) || defined(__clang__)

const int quotient = dividend / divisor;
Expand Down Expand Up @@ -120,11 +119,12 @@ 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
if (__builtin_constant_p(divisor) && isPowerOfTwo(divisor)) return dividend & (divisor - 1);
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);
Expand Down
3 changes: 2 additions & 1 deletion test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ set (GTESTP4C_SOURCES ${GTEST_SOURCES} ${GTEST_BASE_SOURCES})
include_directories(${CMAKE_CURRENT_BINARY_DIR}/test)
configure_file(gtest/env.h.in ${CMAKE_CURRENT_BINARY_DIR}/gtest/env.h)
add_executable (gtestp4c ${GTESTP4C_SOURCES})
target_link_libraries (gtestp4c ${GTEST_LDADD} ${P4C_LIBRARIES} gtest ${P4C_LIB_DEPS})
target_link_libraries (gtestp4c PUBLIC ${GTEST_LDADD} PUBLIC ${P4C_LIBRARIES} PUBLIC gtest PUBLIC ${P4C_LIB_DEPS})
add_dependencies(gtestp4c gtest genIR frontend controlplane)

# The load_ir_from_json test needs this file. Easier to copy to build directory
Expand All @@ -87,3 +87,4 @@ add_dependencies(gtestp4c copy_gtest_deps)
# Tests
add_test (NAME gtestp4c COMMAND gtestp4c WORKING_DIRECTORY ${P4C_BINARY_DIR})
set_tests_properties (gtestp4c PROPERTIES LABELS "gtest")
target_link_libraries(gtestp4c PRIVATE absl::bits)

0 comments on commit 67ac50b

Please sign in to comment.