Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
5 changes: 5 additions & 0 deletions libc/cmake/modules/CheckCompilerFeatures.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ set(
"fixed_point"
"cfloat16"
"cfloat128"
"padding_on_unsigned_fixed_point"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove changes on this file and check_padding_on_unsigned_fixed_point.cpp and leave them for other PR? They won't work as intended without changes in include/llvm-libc-macros/stdfix-macros.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 632a6d8b

)

# Making sure ALL_COMPILER_FEATURES is sorted.
Expand Down Expand Up @@ -64,6 +65,8 @@ foreach(feature IN LISTS ALL_COMPILER_FEATURES)
set(link_options "")
if(${feature} STREQUAL "fixed_point")
list(APPEND compile_options "-ffixed-point")
elseif(${feature} STREQUAL "padding_on_unsigned_fixed_point")
list(APPEND compile_options "-ffixed-point -Xclang=-fpadding-on-unsigned-fixed-point")
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be able to support GCC right? And in general, using -Xclang options is a little dubious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi! i'm having a tough time finding an alternative that would support GCC too, can you please provide some guidance?

Copy link
Contributor

Choose a reason for hiding this comment

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

This option confuses me, it's a language option but it's not enabled by anything but this -cc1 option. Forcing the user to go through -Xclang for anything but hacking on the compiler internals is generally bad. My assumption is that this flag should be promoted to a marshalling driver flag. I don't know what the equivalent in GCC would be, nor why this flag is necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently fixed point is only supported in clang.

Copy link
Member

Choose a reason for hiding this comment

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

I would also highly recommend AVOIDING dependencies on clang or llvm internal flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

This part is to test whether compiler accepting these flags, and both are recently added to clang. @PiJoules might know more about them.

My only concern is that maybe we should only test -Xclang=-fpadding-on-unsigned-fixed-point flag if -ffixed-point is supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah the original intent I had was having a parallel set of tests with this -Xclang flag to ensure more correctness. -Xclang=-fpadding-on-unsigned-fixed-point changes the layout for the unsigned fixed point types but the function should be able to work correctly independent of ABI changes. It's hidden since users generally shouldn't really care about the layout. GCC AFAICT doesn't have an equivalent flag.

If it's recommended that internal llvm flags shouldn't be used, then I'm fine with not having tests with this flag. Otherwise, it I think should also be gated on clang being the compiler, -ffixed-point being suppported, and this should only be limited to tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've removed both the usage of the -Xclang=-fpadding-on-unsigned-fixed-point flag and its associated tests, while retaining support for the underlying functionality for the same in libc/src/__support/fixed_point/fx_bits.h in the fixed_point::countls function. Perhaps we can implement that feature (and tests) in another dedicated PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

elseif(${feature} MATCHES "^builtin_" OR
${feature} STREQUAL "float16_conversion")
set(compile_options ${LIBC_COMPILE_OPTIONS_DEFAULT})
Expand Down Expand Up @@ -112,6 +115,8 @@ foreach(feature IN LISTS ALL_COMPILER_FEATURES)
set(LIBC_TYPES_HAS_FLOAT128 TRUE)
elseif(${feature} STREQUAL "fixed_point")
set(LIBC_COMPILER_HAS_FIXED_POINT TRUE)
elseif(${feature} STREQUAL "padding_on_unsigned_fixed_point")
set(LIBC_COMPILER_HAS_PADDING_ON_UNSIGNED_FIXED_POINT TRUE)
elseif(${feature} STREQUAL "cfloat16")
set(LIBC_TYPES_HAS_CFLOAT16 TRUE)
elseif(${feature} STREQUAL "cfloat128")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#include "include/llvm-libc-macros/stdfix-macros.h"

#ifndef LIBC_COMPILER_HAS_PADDING_ON_UNSIGNED_FIXED_POINT
#error unsupported
#endif
12 changes: 12 additions & 0 deletions libc/config/baremetal/arm/entrypoints.txt
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,18 @@ if(LIBC_COMPILER_HAS_FIXED_POINT)
libc.src.stdfix.ukbits
libc.src.stdfix.lkbits
libc.src.stdfix.ulkbits
libc.src.stdfix.countlshr
libc.src.stdfix.countlsr
libc.src.stdfix.countlslr
libc.src.stdfix.countlshk
libc.src.stdfix.countlsk
libc.src.stdfix.countlslk
libc.src.stdfix.countlsuhr
libc.src.stdfix.countlsur
libc.src.stdfix.countlsulr
libc.src.stdfix.countlsuhk
libc.src.stdfix.countlsuk
libc.src.stdfix.countlsulk
)
endif()

Expand Down
12 changes: 12 additions & 0 deletions libc/config/baremetal/riscv/entrypoints.txt
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,18 @@ if(LIBC_COMPILER_HAS_FIXED_POINT)
libc.src.stdfix.ukbits
libc.src.stdfix.lkbits
libc.src.stdfix.ulkbits
libc.src.stdfix.countlshr
libc.src.stdfix.countlsr
libc.src.stdfix.countlslr
libc.src.stdfix.countlshk
libc.src.stdfix.countlsk
libc.src.stdfix.countlslk
libc.src.stdfix.countlsuhr
libc.src.stdfix.countlsur
libc.src.stdfix.countlsulr
libc.src.stdfix.countlsuhk
libc.src.stdfix.countlsuk
libc.src.stdfix.countlsulk
)
endif()

Expand Down
12 changes: 12 additions & 0 deletions libc/config/linux/riscv/entrypoints.txt
Original file line number Diff line number Diff line change
Expand Up @@ -744,6 +744,18 @@ if(LIBC_COMPILER_HAS_FIXED_POINT)
# TODO: https://github.com/llvm/llvm-project/issues/115778
libc.src.stdfix.lkbits
libc.src.stdfix.ulkbits
libc.src.stdfix.countlshr
libc.src.stdfix.countlsr
libc.src.stdfix.countlslr
libc.src.stdfix.countlshk
libc.src.stdfix.countlsk
libc.src.stdfix.countlslk
libc.src.stdfix.countlsuhr
libc.src.stdfix.countlsur
libc.src.stdfix.countlsulr
libc.src.stdfix.countlsuhk
libc.src.stdfix.countlsuk
libc.src.stdfix.countlsulk
)
endif()

Expand Down
12 changes: 12 additions & 0 deletions libc/config/linux/x86_64/entrypoints.txt
Original file line number Diff line number Diff line change
Expand Up @@ -869,6 +869,18 @@ if(LIBC_COMPILER_HAS_FIXED_POINT)
libc.src.stdfix.ukbits
libc.src.stdfix.lkbits
libc.src.stdfix.ulkbits
libc.src.stdfix.countlshr
libc.src.stdfix.countlsr
libc.src.stdfix.countlslr
libc.src.stdfix.countlshk
libc.src.stdfix.countlsk
libc.src.stdfix.countlslk
libc.src.stdfix.countlsuhr
libc.src.stdfix.countlsur
libc.src.stdfix.countlsulr
libc.src.stdfix.countlsuhk
libc.src.stdfix.countlsuk
libc.src.stdfix.countlsulk
)
endif()

Expand Down
2 changes: 1 addition & 1 deletion libc/docs/headers/math/stdfix.rst
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ The following functions are included in the ISO/IEC TR 18037:2008 standard.
+---------------+----------------+-------------+---------------+------------+----------------+-------------+----------------+-------------+---------------+------------+----------------+-------------+
| \*bits | | | | | | | | | | | | |
+---------------+----------------+-------------+---------------+------------+----------------+-------------+----------------+-------------+---------------+------------+----------------+-------------+
| countls | | | | | | | | | | | | |
| countls | |check| | |check| | |check| | |check| | |check| | |check| | |check| | |check| | |check| | |check| | |check| | |check| |
+---------------+----------------+-------------+---------------+------------+----------------+-------------+----------------+-------------+---------------+------------+----------------+-------------+
| divi | | | | | | | | | | | | |
+---------------+----------------+-------------+---------------+------------+----------------+-------------+----------------+-------------+---------------+------------+----------------+-------------+
Expand Down
84 changes: 84 additions & 0 deletions libc/include/stdfix.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -306,3 +306,87 @@ functions:
arguments:
- type: unsigned int
guard: LIBC_COMPILER_HAS_FIXED_POINT
- name: countlshr
standards:
- stdc_ext
return_type: int
arguments:
- type: short fract
guard: LIBC_COMPILER_HAS_FIXED_POINT
- name: countlsr
standards:
- stdc_ext
return_type: int
arguments:
- type: fract
guard: LIBC_COMPILER_HAS_FIXED_POINT
- name: countlslr
standards:
- stdc_ext
return_type: int
arguments:
- type: long fract
guard: LIBC_COMPILER_HAS_FIXED_POINT
- name: countlshk
standards:
- stdc_ext
return_type: int
arguments:
- type: short accum
guard: LIBC_COMPILER_HAS_FIXED_POINT
- name: countlsk
standards:
- stdc_ext
return_type: int
arguments:
- type: accum
guard: LIBC_COMPILER_HAS_FIXED_POINT
- name: countlslk
standards:
- stdc_ext
return_type: int
arguments:
- type: long accum
guard: LIBC_COMPILER_HAS_FIXED_POINT
- name: countlsuhr
standards:
- stdc_ext
return_type: int
arguments:
- type: unsigned short fract
guard: LIBC_COMPILER_HAS_FIXED_POINT
- name: countlsur
standards:
- stdc_ext
return_type: int
arguments:
- type: unsigned fract
guard: LIBC_COMPILER_HAS_FIXED_POINT
- name: countlsulr
standards:
- stdc_ext
return_type: int
arguments:
- type: unsigned long fract
guard: LIBC_COMPILER_HAS_FIXED_POINT
- name: countlsuhk
standards:
- stdc_ext
return_type: int
arguments:
- type: unsigned short accum
guard: LIBC_COMPILER_HAS_FIXED_POINT
- name: countlsuk
standards:
- stdc_ext
return_type: int
arguments:
- type: unsigned accum
guard: LIBC_COMPILER_HAS_FIXED_POINT
- name: countlsulk
standards:
- stdc_ext
return_type: int
arguments:
- type: unsigned long accum
guard: LIBC_COMPILER_HAS_FIXED_POINT
1 change: 1 addition & 0 deletions libc/src/__support/fixed_point/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ add_header_library(
libc.src.__support.macros.optimization
libc.src.__support.CPP.type_traits
libc.src.__support.CPP.bit
libc.src.__support.CPP.limits
libc.src.__support.math_extras
)

Expand Down
35 changes: 34 additions & 1 deletion libc/src/__support/fixed_point/fx_bits.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@
#include "include/llvm-libc-macros/stdfix-macros.h"
#include "src/__support/CPP/bit.h"
#include "src/__support/CPP/type_traits.h"
#include "src/__support/CPP/limits.h" // numeric_limits
#include "src/__support/macros/attributes.h" // LIBC_INLINE
#include "src/__support/macros/config.h"
#include "src/__support/macros/config.h" // LIBC_NAMESPACE_DECL
#include "src/__support/macros/optimization.h" // LIBC_UNLIKELY
#include "src/__support/math_extras.h"

Expand Down Expand Up @@ -50,6 +51,12 @@ template <typename T> struct FXBits {
static constexpr StorageType SIGN_MASK =
(fx_rep::SIGN_LEN == 0 ? 0 : StorageType(1) << SIGN_OFFSET);

// mask for <integral | fraction>
static constexpr StorageType VALUE_MASK = INTEGRAL_MASK | FRACTION_MASK;

// mask for <sign | integral | fraction>
static constexpr StorageType TOTAL_MASK = SIGN_MASK | VALUE_MASK;

public:
LIBC_INLINE constexpr FXBits() = default;

Expand All @@ -74,6 +81,12 @@ template <typename T> struct FXBits {
return (value & INTEGRAL_MASK) >> INTEGRAL_OFFSET;
}

// returns complete bitstring representation the fixed point number
// the bitstring is of the form: padding | sign | integral | fraction
LIBC_INLINE constexpr StorageType get_bits() {
return (value & TOTAL_MASK) >> FRACTION_OFFSET;
}

// TODO: replace bool with Sign
LIBC_INLINE constexpr bool get_sign() {
return static_cast<bool>((value & SIGN_MASK) >> SIGN_OFFSET);
Expand Down Expand Up @@ -163,6 +176,26 @@ template <typename T> LIBC_INLINE constexpr T round(T x, int n) {
return bit_and((x + round_bit), rounding_mask);
}

// count leading zeros
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably more appropriate to say count leading sign bits to account for negative values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 475945d9

template <typename T>
LIBC_INLINE constexpr cpp::enable_if_t<cpp::is_fixed_point_v<T>, int>
countls(T f) {
Copy link
Member

Choose a reason for hiding this comment

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

libc/src/__support/fixed_point/fx_bits.h:182:11: error: unused parameter 'f' [-Werror,-Wunused-parameter]
  182 | countls(T f) {
      |           ^

guessing x below was supposed to be f?

using FXRep = FXRep<T>;
using BitType = typename FXRep::StorageType;
using FXBits = FXBits<T>;

constexpr int CONTAIN_LEN = cpp::numeric_limits<BitType>::digits;
constexpr int PADDING_LEN = CONTAIN_LEN - (FXRep::INTEGRAL_LEN + FXRep::FRACTION_LEN);
Copy link
Contributor

Choose a reason for hiding this comment

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

Note this should also account for the SIGN_LEN. Right now, this would treat the sign bit as a padding bit, which just so happens to be subtracted from the final calculation so the result of countls is still correct.

I think you could instead use FXRep::TOTAL_LEN here which should account for all the value 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.

fixed in 9b499bcb


if constexpr (FXRep::SIGN_LEN != 0) {
if (x < 0)
Copy link
Member

Choose a reason for hiding this comment

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

From the buildbot failures:

libc/src/__support/fixed_point/fx_bits.h:191:9: error: use of undeclared identifier 'x'
  191 |     if (x < 0)
      |         ^

x = bit_not(x);
}

BitType value_bits = FXBits(x)::get_bits();
return cpp::countl_zero(value_bits) - PADDING_LEN;
}

} // namespace fixed_point
} // namespace LIBC_NAMESPACE_DECL

Expand Down
30 changes: 30 additions & 0 deletions libc/src/stdfix/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -104,3 +104,33 @@ add_entrypoint_object(
libc.src.__support.fixed_point.fx_rep
libc.src.__support.CPP.bit
)


foreach(suffix IN ITEMS hr r lr hk k lk uhr ur ulr uhk uk ulk)
add_entrypoint_object(
countls${suffix}
HDRS
countls${suffix}.h
SRCS
countls${suffix}.cpp
COMPILE_OPTIONS
${libc_opt_high_flag}
DEPENDS
libc.src.__support.fixed_point.fx_bits
)
if(LIBC_COMPILER_HAS_PADDING_ON_UNSIGNED_FIXED_POINT)
add_entrypoint_object(
countls${suffix}_padding_on_unsigned_fixed_point
HDRS
countls${suffix}.h
SRCS
countls${suffix}.cpp
COMPILE_OPTIONS
${libc_opt_high_flag}
-Xclang=-fpadding-on-unsigned-fixed-point
DEPENDS
libc.src.__support.fixed_point.fx_bits
)
endif()

endforeach()
20 changes: 20 additions & 0 deletions libc/src/stdfix/countlshk.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//===-- Implementation for countlshk function --------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include "countlshk.h"
#include "src/__support/common.h"
#include "src/__support/fixed_point/fx_bits.h"
#include "src/__support/macros/config.h" // LIBC_NAMESPACE_DECL

namespace LIBC_NAMESPACE_DECL {

LLVM_LIBC_FUNCTION(int, countlshk, (short accum f)) {
return fixed_point::countls(f);
}

} // namespace LIBC_NAMESPACE_DECL
21 changes: 21 additions & 0 deletions libc/src/stdfix/countlshk.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
//===-- Implementation header for countlshk function -----------*- C++ -*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#ifndef LLVM_LIBC_SRC_STDFIX_COUNTLSHK_H
#define LLVM_LIBC_SRC_STDFIX_COUNTLSHK_H

#include "include/llvm-libc-macros/stdfix-macros.h"
#include "src/__support/macros/config.h" // LIBC_NAMESPACE_DECL

namespace LIBC_NAMESPACE_DECL {

int countlshk(short accum f);

} // namespace LIBC_NAMESPACE_DECL

#endif // LLVM_LIBC_SRC_STDFIX_COUNTLSHK_H
20 changes: 20 additions & 0 deletions libc/src/stdfix/countlshr.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//===-- Implementation for countlshr function --------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#include "countlshr.h"
#include "src/__support/common.h"
#include "src/__support/fixed_point/fx_bits.h"
#include "src/__support/macros/config.h" // LIBC_NAMESPACE_DECL

namespace LIBC_NAMESPACE_DECL {

LLVM_LIBC_FUNCTION(int, countlshr, (short fract f)) {
return fixed_point::countls(f);
}

} // namespace LIBC_NAMESPACE_DECL
21 changes: 21 additions & 0 deletions libc/src/stdfix/countlshr.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
//===-- Implementation header for countlshr function -----------*- C++ -*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#ifndef LLVM_LIBC_SRC_STDFIX_COUNTLSHR_H
#define LLVM_LIBC_SRC_STDFIX_COUNTLSHR_H

#include "include/llvm-libc-macros/stdfix-macros.h"
#include "src/__support/macros/config.h" // LIBC_NAMESPACE_DECL

namespace LIBC_NAMESPACE_DECL {

int countlshr(short fract f);

} // namespace LIBC_NAMESPACE_DECL

#endif // LLVM_LIBC_SRC_STDFIX_COUNTLSHR_H
Loading
Loading