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.

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

Implement fixed point countls functions in llvm-libc #113357

Closed
PiJoules opened this issue Oct 22, 2024 · 10 comments · Fixed by #125356 or #126597
Closed

Implement fixed point countls functions in llvm-libc #113357

PiJoules opened this issue Oct 22, 2024 · 10 comments · Fixed by #125356 or #126597
Labels
clang:headers Headers provided by Clang, e.g. for intrinsics good first issue https://github.com/llvm/llvm-project/contribute libc

Comments

@PiJoules
Copy link
Contributor

PiJoules commented Oct 22, 2024

Some fixed point functions from ISO 18037 are implemented in llvm-libc, but not all of them are implemented. The various countlsfx functions should also be added. Copying from the extension:

4.1.7.3 The fixed-point bit countls functions

The bit count functions countlsfx, where fx stands for one of hr, r, lr, hk, k, lk, uhr, ur, ulr, uhk, uk or ulk, take one fixed-point type argument (corresponding to fx); the result type is int.
The integer return value of the above functions is defined as follows:
- if the value of the fixed-point argument is non-zero, the return value is the largest integer k for
which the expression a<<k does not overflow;
- if the value of the fixed-point argument is zero, an integer value is returned that is at least as large as N, where N is the total number of value bits of the fixed-point type of the argument.

This effectively returns the number of leading sign bits. These can first be implemented as __builtin_* functions in clang then llvm-libc can provide the countlsfx wrappers for each of the builtin functions.

@PiJoules PiJoules added good first issue https://github.com/llvm/llvm-project/contribute clang Clang issues not falling into any other category libc labels Oct 22, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 22, 2024

@llvm/issue-subscribers-libc

Author: None (PiJoules)

Some fixed point functions from ISO 18037 are implemented in llvm-libc, but not all of them are implemented. The various `countls` should also be added. Copying from the extension:
4.1.7.3 The fixed-point bit countls functions

The bit count functions countlsfx, where fx stands for one of hr, r, lr, hk, k, lk, uhr, ur, ulr, uhk, uk or ulk, take one fixed-point type argument (corresponding to fx); the result type is int.
The integer return value of the above functions is defined as follows:
- if the value of the fixed-point argument is non-zero, the return value is the largest integer k for
which the expression a&lt;&lt;k does not overflow;
- if the value of the fixed-point argument is zero, an integer value is returned that is at least as large as N, where N is the total number of value bits of the fixed-point type of the argument.

This effectively returns the number of leading sign bits. These can first be implemented as __builtin_* functions in clang then llvm-libc can provide the countlsfx wrappers for each of the builtin functions.

@llvmbot
Copy link
Member

llvmbot commented Oct 22, 2024

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. Check that no other contributor has already been assigned to this issue. If you believe that no one is actually working on it despite an assignment, ping the person. After one week without a response, the assignee may be changed.
  2. In the comments of this issue, request for it to be assigned to you, or just create a pull request after following the steps below. Mention this issue in the description of the pull request.
  3. Fix the issue locally.
  4. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  5. Create a Git commit.
  6. Run git clang-format HEAD~1 to format your changes.
  7. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation. Mention this issue in the description of the pull request.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

@llvmbot
Copy link
Member

llvmbot commented Oct 22, 2024

@llvm/issue-subscribers-good-first-issue

Author: None (PiJoules)

Some fixed point functions from ISO 18037 are implemented in llvm-libc, but not all of them are implemented. The various `countls` should also be added. Copying from the extension:
4.1.7.3 The fixed-point bit countls functions

The bit count functions countlsfx, where fx stands for one of hr, r, lr, hk, k, lk, uhr, ur, ulr, uhk, uk or ulk, take one fixed-point type argument (corresponding to fx); the result type is int.
The integer return value of the above functions is defined as follows:
- if the value of the fixed-point argument is non-zero, the return value is the largest integer k for
which the expression a&lt;&lt;k does not overflow;
- if the value of the fixed-point argument is zero, an integer value is returned that is at least as large as N, where N is the total number of value bits of the fixed-point type of the argument.

This effectively returns the number of leading sign bits. These can first be implemented as __builtin_* functions in clang then llvm-libc can provide the countlsfx wrappers for each of the builtin functions.

@PiJoules PiJoules changed the title Implement fixed point countlsfx functions in llvm-libc Implement fixed point countls functions in llvm-libc Oct 22, 2024
@EugeneZelenko EugeneZelenko added clang:headers Headers provided by Clang, e.g. for intrinsics and removed clang Clang issues not falling into any other category labels Oct 22, 2024
@duncpro
Copy link
Contributor

duncpro commented Oct 26, 2024

I'll implement this! Please assign.

@duncpro
Copy link
Contributor

duncpro commented Oct 26, 2024

@PiJoules Are we sure we want to implement these as clang builtins? Isn't that bad for portability? We wouldn't be able to build llvm-libc using an alternative compiler like GCC for instance...

https://libc.llvm.org/compiler_support.html

Sorry if this is a dumb question. It's my first contribution to LLVM.

@lntue
Copy link
Contributor

lntue commented Oct 29, 2024

@PiJoules Are we sure we want to implement these as clang builtins? Isn't that bad for portability? We wouldn't be able to build llvm-libc using an alternative compiler like GCC for instance...

https://libc.llvm.org/compiler_support.html

Sorry if this is a dumb question. It's my first contribution to LLVM.

Currently generic versions of gcc do not support fixed point types.

OTOH, since C23 defines many count / bit functions for unsigned integers under the names: stdc_leading_zeros_* and stdc_leading_ones_* (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf - 7.18.3 & 7.18.4) (c.f. https://github.com/llvm/llvm-project/blob/main/libc/src/stdbit/stdc_count_ones_uc.cpp ),
maybe we should add stdc_leading_ones_u* for fixed point types using cpp::bit_cast<...>(...) and current implementations of those functions for integers. Then you can implement countls for fixed point without adding new builtins.

Also we probably should post a RFC on LLVM discourse to consult with some C language standard people about modernizing the name of countls to be close to current C23 naming choices.

@PiJoules
Copy link
Contributor Author

I'd also support the stdc_leading_ones/zeros approach and reuse those for fixed point types. The __builtin_ approach was just a suggestion since the compiler already knows the bit layout of the types and can "just emit" the right IR for each builtin call. Clang already does this for other ones like __builtin_clrsb or __builtin_clz. These should also be doable in libc since the layout should be exposed via the macros shown in stdfix-macros.h.

Thanks for taking this on also!

@erichkeane
Copy link
Collaborator

@duncpro deleted the original attempt at a fix, so marking this as unassigned so that someone else can pick up the effort.

@krishna2803
Copy link
Contributor

hi @erichkeane i would like to work on this

@erichkeane
Copy link
Collaborator

Great! Let us know if you need help along the way, and let us know when you've posted a patch.

Icohedron pushed a commit to Icohedron/llvm-project that referenced this issue Feb 11, 2025
lntue pushed a commit that referenced this issue Feb 12, 2025
flovent pushed a commit to flovent/llvm-project that referenced this issue Feb 13, 2025
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this issue Feb 14, 2025
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this issue Feb 24, 2025
YutongZhuu pushed a commit to YutongZhuu/llvm-project that referenced this issue Mar 8, 2025
YutongZhuu pushed a commit to YutongZhuu/llvm-project that referenced this issue Mar 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:headers Headers provided by Clang, e.g. for intrinsics good first issue https://github.com/llvm/llvm-project/contribute libc
Projects
None yet
7 participants