[PowerPC] Emit libcall to frexpl for calls to frexp(ppcDoublDouble) #75226

lei137 · 2023-12-12T17:49:21Z

On Linux PPC call lib func frexpl for calls to frexp() for input of type PPCDoubleDouble.

Fixes bug: #64426

llvmbot · 2023-12-12T17:49:48Z

@llvm/pr-subscribers-libcxx

@llvm/pr-subscribers-clang-codegen

Author: Lei Huang (lei137)

Changes

On Linux PPC call lib func frexpl for calls to frexp() for input of type PPCDoubleDouble.

Fixes bug: #64426

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

2 Files Affected:

(modified) clang/lib/CodeGen/CGBuiltin.cpp (+15-1)
(modified) libcxx/test/libcxx/numerics/c.math/constexpr-cxx23-clang.pass.cpp (-6)

diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 83d0a72aac5495..7a700155149e8f 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -137,6 +137,10 @@ llvm::Constant *CodeGenModule::getBuiltinLibFunction(const FunctionDecl *FD,
       {Builtin::BI__builtin_modfl, "modf"},
   };
 
+  static SmallDenseMap<unsigned, StringRef, 4> PPCDoubleDoubleBuiltins{
+      {Builtin::BI__builtin_frexpl, "frexpl"},
+  };
+
   // If the builtin has been declared explicitly with an assembler label,
   // use the mangled name. This differs from the plain label on platforms
   // that prefix labels.
@@ -149,6 +153,10 @@ llvm::Constant *CodeGenModule::getBuiltinLibFunction(const FunctionDecl *FD,
         &getTarget().getLongDoubleFormat() == &llvm::APFloat::IEEEquad() &&
         F128Builtins.contains(BuiltinID))
       Name = F128Builtins[BuiltinID];
+    else if (getTriple().isPPC() && getTriple().isOSLinux() &&
+        &getTarget().getLongDoubleFormat() == &llvm::APFloat::PPCDoubleDouble()
+        && PPCDoubleDoubleBuiltins.contains(BuiltinID))
+      Name = PPCDoubleDoubleBuiltins[BuiltinID];
     else if (getTriple().isOSAIX() &&
              &getTarget().getLongDoubleFormat() ==
                  &llvm::APFloat::IEEEdouble() &&
@@ -3410,9 +3418,15 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
                                    { Src0->getType(), Src1->getType() });
     return RValue::get(Builder.CreateCall(F, { Src0, Src1 }));
   }
+  case Builtin::BI__builtin_frexpl: {
+    auto &Triple = getTarget().getTriple();
+    if (Triple.isPPC() && Triple.isOSLinux() &&
+        &getTarget().getLongDoubleFormat() == &llvm::APFloat::PPCDoubleDouble())
+      break;
+    LLVM_FALLTHROUGH;
+  }
   case Builtin::BI__builtin_frexp:
   case Builtin::BI__builtin_frexpf:
-  case Builtin::BI__builtin_frexpl:
   case Builtin::BI__builtin_frexpf128:
   case Builtin::BI__builtin_frexpf16:
     return RValue::get(emitFrexpBuiltin(*this, E, Intrinsic::frexp));
diff --git a/libcxx/test/libcxx/numerics/c.math/constexpr-cxx23-clang.pass.cpp b/libcxx/test/libcxx/numerics/c.math/constexpr-cxx23-clang.pass.cpp
index 31511064ce7ca5..a07260a34516f1 100644
--- a/libcxx/test/libcxx/numerics/c.math/constexpr-cxx23-clang.pass.cpp
+++ b/libcxx/test/libcxx/numerics/c.math/constexpr-cxx23-clang.pass.cpp
@@ -58,15 +58,9 @@ int main(int, char**) {
 
   ASSERT_NOT_CONSTEXPR_CXX23(std::frexp(0.0f, &DummyInt) == 0.0f);
   ASSERT_NOT_CONSTEXPR_CXX23(std::frexp(0.0, &DummyInt) == 0.0);
-//FIXME: currently linux powerpc does not support this expansion
-// since 0.0L lowers to ppcf128 and special handling is required.
-#if !defined(__LONG_DOUBLE_IBM128__)
   ASSERT_NOT_CONSTEXPR_CXX23(std::frexp(0.0L, &DummyInt) == 0.0L);
-#endif
   ASSERT_NOT_CONSTEXPR_CXX23(std::frexpf(0.0f, &DummyInt) == 0.0f);
-#if !defined(__LONG_DOUBLE_IBM128__)
   ASSERT_NOT_CONSTEXPR_CXX23(std::frexpl(0.0L, &DummyInt) == 0.0L);
-#endif
 
   ASSERT_NOT_CONSTEXPR_CXX23(std::ilogb(1.0f) == 0);
   ASSERT_NOT_CONSTEXPR_CXX23(std::ilogb(1.0) == 0);

llvmbot · 2023-12-12T17:49:48Z

@llvm/pr-subscribers-clang

Author: Lei Huang (lei137)

Changes

On Linux PPC call lib func frexpl for calls to frexp() for input of type PPCDoubleDouble.

Fixes bug: #64426

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

2 Files Affected:

(modified) clang/lib/CodeGen/CGBuiltin.cpp (+15-1)
(modified) libcxx/test/libcxx/numerics/c.math/constexpr-cxx23-clang.pass.cpp (-6)

diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 83d0a72aac549..7a700155149e8 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -137,6 +137,10 @@ llvm::Constant *CodeGenModule::getBuiltinLibFunction(const FunctionDecl *FD,
       {Builtin::BI__builtin_modfl, "modf"},
   };
 
+  static SmallDenseMap<unsigned, StringRef, 4> PPCDoubleDoubleBuiltins{
+      {Builtin::BI__builtin_frexpl, "frexpl"},
+  };
+
   // If the builtin has been declared explicitly with an assembler label,
   // use the mangled name. This differs from the plain label on platforms
   // that prefix labels.
@@ -149,6 +153,10 @@ llvm::Constant *CodeGenModule::getBuiltinLibFunction(const FunctionDecl *FD,
         &getTarget().getLongDoubleFormat() == &llvm::APFloat::IEEEquad() &&
         F128Builtins.contains(BuiltinID))
       Name = F128Builtins[BuiltinID];
+    else if (getTriple().isPPC() && getTriple().isOSLinux() &&
+        &getTarget().getLongDoubleFormat() == &llvm::APFloat::PPCDoubleDouble()
+        && PPCDoubleDoubleBuiltins.contains(BuiltinID))
+      Name = PPCDoubleDoubleBuiltins[BuiltinID];
     else if (getTriple().isOSAIX() &&
              &getTarget().getLongDoubleFormat() ==
                  &llvm::APFloat::IEEEdouble() &&
@@ -3410,9 +3418,15 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
                                    { Src0->getType(), Src1->getType() });
     return RValue::get(Builder.CreateCall(F, { Src0, Src1 }));
   }
+  case Builtin::BI__builtin_frexpl: {
+    auto &Triple = getTarget().getTriple();
+    if (Triple.isPPC() && Triple.isOSLinux() &&
+        &getTarget().getLongDoubleFormat() == &llvm::APFloat::PPCDoubleDouble())
+      break;
+    LLVM_FALLTHROUGH;
+  }
   case Builtin::BI__builtin_frexp:
   case Builtin::BI__builtin_frexpf:
-  case Builtin::BI__builtin_frexpl:
   case Builtin::BI__builtin_frexpf128:
   case Builtin::BI__builtin_frexpf16:
     return RValue::get(emitFrexpBuiltin(*this, E, Intrinsic::frexp));
diff --git a/libcxx/test/libcxx/numerics/c.math/constexpr-cxx23-clang.pass.cpp b/libcxx/test/libcxx/numerics/c.math/constexpr-cxx23-clang.pass.cpp
index 31511064ce7ca..a07260a34516f 100644
--- a/libcxx/test/libcxx/numerics/c.math/constexpr-cxx23-clang.pass.cpp
+++ b/libcxx/test/libcxx/numerics/c.math/constexpr-cxx23-clang.pass.cpp
@@ -58,15 +58,9 @@ int main(int, char**) {
 
   ASSERT_NOT_CONSTEXPR_CXX23(std::frexp(0.0f, &DummyInt) == 0.0f);
   ASSERT_NOT_CONSTEXPR_CXX23(std::frexp(0.0, &DummyInt) == 0.0);
-//FIXME: currently linux powerpc does not support this expansion
-// since 0.0L lowers to ppcf128 and special handling is required.
-#if !defined(__LONG_DOUBLE_IBM128__)
   ASSERT_NOT_CONSTEXPR_CXX23(std::frexp(0.0L, &DummyInt) == 0.0L);
-#endif
   ASSERT_NOT_CONSTEXPR_CXX23(std::frexpf(0.0f, &DummyInt) == 0.0f);
-#if !defined(__LONG_DOUBLE_IBM128__)
   ASSERT_NOT_CONSTEXPR_CXX23(std::frexpl(0.0L, &DummyInt) == 0.0L);
-#endif
 
   ASSERT_NOT_CONSTEXPR_CXX23(std::ilogb(1.0f) == 0);
   ASSERT_NOT_CONSTEXPR_CXX23(std::ilogb(1.0) == 0);

github-actions · 2023-12-12T17:51:43Z

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

efriedma-quic · 2023-12-12T18:22:28Z

How hard is it to fix the backend to handle this properly? We should already have support for other libcalls, I think. I'd like to avoid adding temporary hacks to clang.

ldionne

Just approving the libc++ part of the change, which is not controversial. Please work with the other reviewers for the rest of the patch. Thanks for working on this!

lei137 · 2023-12-12T20:54:36Z

How hard is it to fix the backend to handle this properly? We should already have support for other libcalls, I think. I'd like to avoid adding temporary hacks to clang.

@efriedma-quic The original failure is becaues we are producing illegal types after type legalization (i.e. during operation legalization). To fix this in the backend we need to properly handle PPC double-double. I've spoken with the PPC code owner and it was decided that since PPC double-double is a dying type and we are not looking to add more support for it, we would just fix this in the front-end. Work is currently underway to switch to use IEEE Double instead of PPC Double Double by default on PPC.

This failure was introduced in https://reviews.llvm.org/D136538 and wasn't caught for PPC since we did not have libc++ builds as part of our bots. That issue have been addressed and our bots now include building of libc++.

Hopefully this is something that is acceptable to you.

efriedma-quic

If the PPC backend maintainers intend to deprecate ppc-long-double support in LLVM, I guess we can hack clang to limp along until the cut happens.

efriedma-quic · 2023-12-13T01:33:07Z

clang/lib/CodeGen/CGBuiltin.cpp

+             &getTarget().getLongDoubleFormat() ==
+                 &llvm::APFloat::PPCDoubleDouble() &&
+             PPCDoubleDoubleBuiltins.contains(BuiltinID))
+      Name = PPCDoubleDoubleBuiltins[BuiltinID];


Does adding this codepath actually change the computed name here? The default codepath should just chop off the beginning of the string "__builtin_frexpl" to produce the same thing.

I didn't realize that. Thanks for pointing it out.

efriedma-quic · 2023-12-13T01:33:24Z

clang/lib/CodeGen/CGBuiltin.cpp

@@ -3410,9 +3419,15 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
                                   { Src0->getType(), Src1->getType() });
    return RValue::get(Builder.CreateCall(F, { Src0, Src1 }));
  }
+  case Builtin::BI__builtin_frexpl: {
+    auto &Triple = getTarget().getTriple();
+    if (Triple.isPPC() && Triple.isOSLinux() &&


Checking isOSLinux() seems more likely to cause trouble than to solve anything; checking the floating-point format should be sufficient to ensure this doesn't trigger by accident.

Maybe add a brief comment here explaining why this special case is necessary.

efriedma-quic

LGTM

On Linux PPC call lib func ``frexpl`` for calls to ``frexp()`` for input of type PPCDoubleDouble. Fixes bug: llvm#64426

lei137 requested a review from a team as a code owner December 12, 2023 17:49

lei137 self-assigned this Dec 12, 2023

llvmbot added clang Clang issues not falling into any other category libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. clang:codegen IR generation bugs: mangling, exceptions, etc. labels Dec 12, 2023

lei137 requested review from stefanp-synopsys and nemanjai December 12, 2023 17:49

lei137 force-pushed the bug67735 branch from b815b9b to 2e6a96f Compare December 12, 2023 17:59

ldionne approved these changes Dec 12, 2023

View reviewed changes

efriedma-quic reviewed Dec 13, 2023

View reviewed changes

lei137 requested a review from efriedma-quic December 14, 2023 15:30

lei137 force-pushed the bug67735 branch from 1c87d4d to b5dbfff Compare December 14, 2023 17:39

efriedma-quic approved these changes Dec 15, 2023

View reviewed changes

lei137 added 3 commits December 15, 2023 15:08

[PowerPC] Emit libcall to frexpl for calls to frexp(ppcDoublDouble)

f38e709

On Linux PPC call lib func ``frexpl`` for calls to ``frexp()`` for input of type PPCDoubleDouble. Fixes bug: llvm#64426

address Eli's review comments

3065e12

fixup condition

7638440

lei137 force-pushed the bug67735 branch from b5dbfff to 7638440 Compare December 15, 2023 21:08

lei137 merged commit aaa3f72 into llvm:main Dec 15, 2023

lei137 deleted the bug67735 branch December 15, 2023 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PowerPC] Emit libcall to frexpl for calls to frexp(ppcDoublDouble) #75226

[PowerPC] Emit libcall to frexpl for calls to frexp(ppcDoublDouble) #75226

lei137 commented Dec 12, 2023

llvmbot commented Dec 12, 2023 •

edited

Loading

llvmbot commented Dec 12, 2023

github-actions bot commented Dec 12, 2023 •

edited

Loading

efriedma-quic commented Dec 12, 2023

ldionne left a comment

lei137 commented Dec 12, 2023

efriedma-quic left a comment

efriedma-quic Dec 13, 2023

lei137 Dec 13, 2023

efriedma-quic Dec 13, 2023

efriedma-quic left a comment

[PowerPC] Emit libcall to frexpl for calls to frexp(ppcDoublDouble) #75226

[PowerPC] Emit libcall to frexpl for calls to frexp(ppcDoublDouble) #75226

Conversation

lei137 commented Dec 12, 2023

llvmbot commented Dec 12, 2023 • edited Loading

llvmbot commented Dec 12, 2023

github-actions bot commented Dec 12, 2023 • edited Loading

efriedma-quic commented Dec 12, 2023

ldionne left a comment

Choose a reason for hiding this comment

lei137 commented Dec 12, 2023

efriedma-quic left a comment

Choose a reason for hiding this comment

efriedma-quic Dec 13, 2023

Choose a reason for hiding this comment

lei137 Dec 13, 2023

Choose a reason for hiding this comment

efriedma-quic Dec 13, 2023

Choose a reason for hiding this comment

efriedma-quic left a comment

Choose a reason for hiding this comment

llvmbot commented Dec 12, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023 •

edited

Loading