[PATCH] D137986: [Clang][CodeGen][AIX] Map __builtin_frexpl, __builtin_ldexpl, and __builtin_modfl to 'double' version lib calls in 64-bit 'long double' mode

[Xing Xue via Phabricator via cfe-commits]

xingxue added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:110
+  // The AIX library functions frexpl, ldexpl, and modfl are for 128-bit
+  // 'long double'. Map to the 'double' versions if it is 64-bit 'long
+  // double' mode.
----------------
daltenty wrote:
> I feel like we should be clear about which 128-bit double format we are talking about, since it may not be immediately clear for folks who aren't intimately familiar with AIX. Maybe we can reference __ibm128 or ibm 128-bit, so it's clear what we mean.
Modified as suggested, thanks!


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:112
+  // double' mode.
+  static SmallDenseMap<unsigned, StringRef, 4> AIXLongDoubleBuiltins{
+      {Builtin::BI__builtin_frexpl, "frexp"},
----------------
hubert.reinterpretcast wrote:
> Please rename to "AIXLongDouble64Builtins".
Renamed as suggested, thanks!


================
Comment at: clang/test/CodeGen/aix-builtin-mapping.c:2
+// AIX library functions frexpl, ldexpl, and modfl are for 128-bit
+// 'long double'. Check that the compiler generates calls to the 'double'
+// versions for correspoding builtin functions in 64-bit 'long double' mode.
----------------
daltenty wrote:
> 
> 
Modified as suggested, thanks!


================
Comment at: clang/test/CodeGen/aix-builtin-mapping.c:3
+// 'long double'. Check that the compiler generates calls to the 'double'
+// versions for correspoding builtin functions in 64-bit 'long double' mode.
+
----------------
hubert.reinterpretcast wrote:
> Fix typo.
Fixed, thanks for catching it!


================
Comment at: clang/test/CodeGen/aix-builtin-mapping.c:5-6
+
+// RUN: %clang_cc1 -triple powerpc-ibm-aix -mlong-double-64 -S -o - %s | FileCheck %s -check-prefix=CHECK
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix -mlong-double-64 -S -o - %s | FileCheck %s -check-prefix=CHECK
+
----------------
hubert.reinterpretcast wrote:
> This should be an IR test. Checking assembly means actually needing the PowerPC target enabled in the LLVM configuration.
Changed to use IR, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137986/new/

https://reviews.llvm.org/D137986