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

[libc][stdfix] Implement countlsfx functions in libc. #126597

Merged
merged 21 commits into from
Feb 12, 2025

Conversation

krishna2803
Copy link
Contributor

fixes #113357

Copy link

github-actions bot commented Feb 10, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@krishna2803
Copy link
Contributor Author

@PiJoules @nickdesaulniers @lntue please give a final review.

@PiJoules
Copy link
Contributor

Just to double check, were you able to build and run these tests locally?

@krishna2803
Copy link
Contributor Author

Yeah, I did the testing, the build succeeded locally (along with the check-libc tests). Just to be absolutely sure, I have just now started yet another build that should probably take an hour or so, I'll update once it's completed.

On a side node, I think we should probably add pre-commit checks for stdfix functions too.

Copy link
Contributor

@PiJoules PiJoules left a comment

Choose a reason for hiding this comment

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

LGTM pending libc owners approval

@krishna2803
Copy link
Contributor Author

Tested again, build was successful.

SRCS
countls${suffix}_test.cpp
COMPILE_OPTIONS
-O3
Copy link
Member

Choose a reason for hiding this comment

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

I think we just removed these

7af4ab4

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 here:

SRCS
countls${suffix}_test.cpp
COMPILE_OPTIONS
${libc_opt_high_flag}

Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

Looks like this fixes the two typos from last time.

Signed-off-by: krishna2803 <[email protected]>
if (f < 0)
f = bit_not(f);
}
Copy link
Member

Choose a reason for hiding this comment

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

ah, you had this correct before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry if im missing something, but the doc says:

// Use braces on the outer `if` to avoid a potential dangling `else`
// situation.
if (isa<VarDecl>(D)) {
  if (shouldProcessAttr(A))
    handleAttr(A);
}

Copy link
Member

Choose a reason for hiding this comment

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

oh, that's news to me. I guess because of constexpr we cant make it if (FXRep::SIGN_LEN > 0 && f < 0)... or could we?

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'll just test it and see if it works!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and it seems it doesn't..

// clang++ -std=c++20 -Wall hello.cpp -o hello
#include <iostream>

constexpr int x = 1;

int main() {
  int y;
  std::cin >> y;

  if constexpr (x > 0 && y > 4)
    std::cout << "foo bar";
}
hello.cpp:10:17: error: constexpr if condition is not a constant expression
  if constexpr (x > 0 && y > 4)
                ^~~~~~~~~~~~~~
hello.cpp:10:26: note: read of non-const variable 'y' is not allowed in a constant expression
  if constexpr (x > 0 && y > 4)
                         ^
hello.cpp:7:7: note: declared here
  int y;
      ^
1 error generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here:

In file included from /home/krishna/OpenSource/llvm-project/libc/src/stdfix/countlsr.cpp:11:
/home/krishna/OpenSource/llvm-project/libc/src/__support/fixed_point/fx_bits.h:188:17: error: constexpr if condition is not a constant expression
  188 |   if constexpr (FXRep::SIGN_LEN > 0 && f < 0)
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/krishna/OpenSource/llvm-project/libc/src/stdfix/countlsr.cpp:16:68: note: in instantiation of function template specialization '__llvm_libc_21_0_0_git::fixed_point::countls<_Fract>' requested here
   16 | LLVM_LIBC_FUNCTION(int, countlsr, (fract f)) { return fixed_point::countls(f); }
      |                                                                    ^
/home/krishna/OpenSource/llvm-project/libc/src/__support/fixed_point/fx_bits.h:188:40: note: function parameter 'f' with unknown value cannot be used in a constant expression
  188 |   if constexpr (FXRep::SIGN_LEN > 0 && f < 0)
      |                                        ^
/home/krishna/OpenSource/llvm-project/libc/src/__support/fixed_point/fx_bits.h:183:11: note: declared here
  183 | countls(T f) {
      |           ^
1 error generated.

@lntue lntue merged commit 1f51038 into llvm:main Feb 12, 2025
14 checks passed
flovent pushed a commit to flovent/llvm-project that referenced this pull request Feb 13, 2025
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Feb 14, 2025
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Feb 24, 2025
YutongZhuu pushed a commit to YutongZhuu/llvm-project that referenced this pull request Mar 8, 2025
YutongZhuu pushed a commit to YutongZhuu/llvm-project that referenced this pull request Mar 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement fixed point countls functions in llvm-libc
4 participants