Reland "[clang] Lower modf builtin using `llvm.modf` intrinsic" #129885

MacDue · 2025-03-05T13:51:28Z

Original description:
This updates the existing modf[f|l] builtin to be lowered via the llvm.modf.* intrinsic (rather than directly to a library call).

The legalization issues exposed by the original PR (#126750) should have been fixed in #128055 and #129264.

…c" (#127…" This reverts commit d595fc9.

llvmbot · 2025-03-05T13:52:02Z

@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Benjamin Maxwell (MacDue)

Changes

Reverts llvm/llvm-project#127987

Original description:
This updates the existing modf[f|l] builtin to be lowered via the llvm.modf.* intrinsic (rather than directly to a library call).

The legalization issues exposed by the original PR (#126750) should have been fixed in #128055 and #129264.

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

6 Files Affected:

(modified) clang/lib/CodeGen/CGBuiltin.cpp (+27)
(modified) clang/test/CodeGen/X86/math-builtins.c (+25-7)
(modified) clang/test/CodeGen/aix-builtin-mapping.c (+1-1)
(modified) clang/test/CodeGen/builtin-attributes.c (+8-3)
(modified) clang/test/CodeGen/math-builtins-long.c (+3-3)
(modified) clang/test/CodeGen/math-libcalls.c (+6-6)

diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index ab8f19b25fa66..bd559a96d3182 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -859,6 +859,24 @@ static void emitSincosBuiltin(CodeGenFunction &CGF, const CallExpr *E,
   StoreCos->setMetadata(LLVMContext::MD_noalias, AliasScopeList);
 }
 
+static llvm::Value *emitModfBuiltin(CodeGenFunction &CGF, const CallExpr *E,
+                                    llvm::Intrinsic::ID IntrinsicID) {
+  llvm::Value *Val = CGF.EmitScalarExpr(E->getArg(0));
+  llvm::Value *IntPartDest = CGF.EmitScalarExpr(E->getArg(1));
+
+  llvm::Value *Call =
+      CGF.Builder.CreateIntrinsic(IntrinsicID, {Val->getType()}, Val);
+
+  llvm::Value *FractionalResult = CGF.Builder.CreateExtractValue(Call, 0);
+  llvm::Value *IntegralResult = CGF.Builder.CreateExtractValue(Call, 1);
+
+  QualType DestPtrType = E->getArg(1)->getType()->getPointeeType();
+  LValue IntegralLV = CGF.MakeNaturalAlignAddrLValue(IntPartDest, DestPtrType);
+  CGF.EmitStoreOfScalar(IntegralResult, IntegralLV);
+
+  return FractionalResult;
+}
+
 /// EmitFAbs - Emit a call to @llvm.fabs().
 static Value *EmitFAbs(CodeGenFunction &CGF, Value *V) {
   Function *F = CGF.CGM.getIntrinsic(Intrinsic::fabs, V->getType());
@@ -4120,6 +4138,15 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
   case Builtin::BI__builtin_frexpf128:
   case Builtin::BI__builtin_frexpf16:
     return RValue::get(emitFrexpBuiltin(*this, E, Intrinsic::frexp));
+  case Builtin::BImodf:
+  case Builtin::BImodff:
+  case Builtin::BImodfl:
+  case Builtin::BI__builtin_modf:
+  case Builtin::BI__builtin_modff:
+  case Builtin::BI__builtin_modfl:
+    if (Builder.getIsFPConstrained())
+      break; // TODO: Emit constrained modf intrinsic once one exists.
+    return RValue::get(emitModfBuiltin(*this, E, Intrinsic::modf));
   case Builtin::BI__builtin_isgreater:
   case Builtin::BI__builtin_isgreaterequal:
   case Builtin::BI__builtin_isless:
diff --git a/clang/test/CodeGen/X86/math-builtins.c b/clang/test/CodeGen/X86/math-builtins.c
index 481d3c043683e..8a85d1f6c3a76 100644
--- a/clang/test/CodeGen/X86/math-builtins.c
+++ b/clang/test/CodeGen/X86/math-builtins.c
@@ -38,6 +38,24 @@ void foo(double *d, float f, float *fp, long double *l, int *i, const char *c) {
 // NO__ERRNO-NEXT: [[FREXP_F128_0:%.+]] = extractvalue { fp128, i32 } [[FREXP_F128]], 0
 
 
+// NO__ERRNO: [[MODF_F64:%.+]] = call { double, double } @llvm.modf.f64(double %{{.+}})
+// NO__ERRNO-NEXT: [[MODF_F64_FP:%.+]] = extractvalue { double, double } [[MODF_F64]], 0
+// NO__ERRNO-NEXT: [[MODF_F64_IP:%.+]] = extractvalue { double, double } [[MODF_F64]], 1
+// NO__ERRNO-NEXT: store double [[MODF_F64_IP]], ptr %{{.+}}, align 8
+
+// NO__ERRNO: [[MODF_F32:%.+]] = call { float, float } @llvm.modf.f32(float %{{.+}})
+// NO__ERRNO-NEXT: [[MODF_F32_FP:%.+]] = extractvalue { float, float } [[MODF_F32]], 0
+// NO__ERRNO-NEXT: [[MODF_F32_IP:%.+]] = extractvalue { float, float } [[MODF_F32]], 1
+// NO__ERRNO-NEXT: store float [[MODF_F32_IP]], ptr %{{.+}}, align 4
+
+// NO__ERRNO: [[MODF_F80:%.+]] = call { x86_fp80, x86_fp80 } @llvm.modf.f80(x86_fp80 %{{.+}})
+// NO__ERRNO-NEXT: [[MODF_F80_FP:%.+]] = extractvalue { x86_fp80, x86_fp80 } [[MODF_F80]], 0
+// NO__ERRNO-NEXT: [[MODF_F80_IP:%.+]] = extractvalue { x86_fp80, x86_fp80 } [[MODF_F80]], 1
+// NO__ERRNO-NEXT: store x86_fp80 [[MODF_F80_IP]], ptr %{{.+}}, align 16
+
+// NO__ERRNO: call fp128 @modff128(fp128 noundef %{{.+}}, ptr noundef %{{.+}})
+
+
 // NO__ERRNO: [[SINCOS_F64:%.+]] = call { double, double } @llvm.sincos.f64(double %{{.+}})
 // NO__ERRNO-NEXT: [[SINCOS_F64_0:%.+]] = extractvalue { double, double } [[SINCOS_F64]], 0
 // NO__ERRNO-NEXT: [[SINCOS_F64_1:%.+]] = extractvalue { double, double } [[SINCOS_F64]], 1
@@ -158,13 +176,13 @@ void foo(double *d, float f, float *fp, long double *l, int *i, const char *c) {
 
   __builtin_modf(f,d);       __builtin_modff(f,fp);      __builtin_modfl(f,l); __builtin_modff128(f,l);
 
-// NO__ERRNO: declare double @modf(double noundef, ptr noundef) [[NOT_READNONE:#[0-9]+]]
-// NO__ERRNO: declare float @modff(float noundef, ptr noundef) [[NOT_READNONE]]
-// NO__ERRNO: declare x86_fp80 @modfl(x86_fp80 noundef, ptr noundef) [[NOT_READNONE]]
-// NO__ERRNO: declare fp128 @modff128(fp128 noundef, ptr noundef) [[NOT_READNONE]]
-// HAS_ERRNO: declare double @modf(double noundef, ptr noundef) [[NOT_READNONE]]
-// HAS_ERRNO: declare float @modff(float noundef, ptr noundef) [[NOT_READNONE]]
-// HAS_ERRNO: declare x86_fp80 @modfl(x86_fp80 noundef, ptr noundef) [[NOT_READNONE]]
+// NO__ERRNO: declare { double, double } @llvm.modf.f64(double) [[READNONE_INTRINSIC]]
+// NO__ERRNO: declare { float, float } @llvm.modf.f32(float) [[READNONE_INTRINSIC]]
+// NO__ERRNO: declare { x86_fp80, x86_fp80 } @llvm.modf.f80(x86_fp80) [[READNONE_INTRINSIC]]
+// NO__ERRNO: declare fp128 @modff128(fp128 noundef, ptr noundef) [[NOT_READNONE:#[0-9]+]]
+// HAS_ERRNO: declare { double, double } @llvm.modf.f64(double) [[READNONE_INTRINSIC]]
+// HAS_ERRNO: declare { float, float } @llvm.modf.f32(float) [[READNONE_INTRINSIC]]
+// HAS_ERRNO: declare { x86_fp80, x86_fp80 } @llvm.modf.f80(x86_fp80) [[READNONE_INTRINSIC]]
 // HAS_ERRNO: declare fp128 @modff128(fp128 noundef, ptr noundef) [[NOT_READNONE]]
 
   __builtin_nan(c);        __builtin_nanf(c);       __builtin_nanl(c); __builtin_nanf128(c);
diff --git a/clang/test/CodeGen/aix-builtin-mapping.c b/clang/test/CodeGen/aix-builtin-mapping.c
index a79218c6f1d8b..cc1cc1a44f32c 100644
--- a/clang/test/CodeGen/aix-builtin-mapping.c
+++ b/clang/test/CodeGen/aix-builtin-mapping.c
@@ -17,6 +17,6 @@ int main()
   returnValue = __builtin_ldexpl(1.0L, 1);
 }
 
-// CHECK: %call = call double @modf(double noundef 1.000000e+00, ptr noundef %DummyLongDouble) #3
+// CHECK: %{{.+}} = call { double, double } @llvm.modf.f64(double 1.000000e+00)
 // CHECK: %{{.+}} = call { double, i32 } @llvm.frexp.f64.i32(double 0.000000e+00)
 // CHECK: %{{.+}} = call double @llvm.ldexp.f64.i32(double 1.000000e+00, i32 1)
diff --git a/clang/test/CodeGen/builtin-attributes.c b/clang/test/CodeGen/builtin-attributes.c
index e5b0faccfd23f..506b165fcf36e 100644
--- a/clang/test/CodeGen/builtin-attributes.c
+++ b/clang/test/CodeGen/builtin-attributes.c
@@ -24,6 +24,11 @@ char* f2(char* a, char* b) {
   return __builtin_strstr(a, b);
 }
 
+// Note: Use asm label to disable intrinsic lowering of modf.
+double modf(double x, double*) asm("modf");
+float modff(float x, float*) asm("modff");
+long double modfl(long double x, long double*) asm("modfl");
+
 // frexp is NOT readnone. It writes to its pointer argument.
 //
 // CHECK: f3
@@ -55,9 +60,9 @@ int f3(double x) {
   frexp(x, &e);
   frexpf(x, &e);
   frexpl(x, &e);
-  __builtin_modf(x, &e);
-  __builtin_modff(x, &e);
-  __builtin_modfl(x, &e);
+  modf(x, &e);
+  modff(x, &e);
+  modfl(x, &e);
   __builtin_remquo(x, x, &e);
   __builtin_remquof(x, x, &e);
   __builtin_remquol(x, x, &e);
diff --git a/clang/test/CodeGen/math-builtins-long.c b/clang/test/CodeGen/math-builtins-long.c
index 183349e0f0173..87e64a2eaa1c3 100644
--- a/clang/test/CodeGen/math-builtins-long.c
+++ b/clang/test/CodeGen/math-builtins-long.c
@@ -58,9 +58,9 @@ void foo(long double f, long double *l, int *i, const char *c) {
   // PPCF128: call fp128 @ldexpf128(fp128 noundef %{{.+}}, {{(signext)?.+}})
   __builtin_ldexpl(f,f);
 
-  // F80: call x86_fp80 @modfl(x86_fp80 noundef %{{.+}}, ptr noundef %{{.+}})
-  // PPC: call ppc_fp128 @modfl(ppc_fp128 noundef %{{.+}}, ptr noundef %{{.+}})
-  // X86F128: call fp128 @modfl(fp128 noundef %{{.+}}, ptr noundef %{{.+}})
+  // F80: call { x86_fp80, x86_fp80 } @llvm.modf.f80(x86_fp80 %{{.+}})
+  // PPC: call { ppc_fp128, ppc_fp128 } @llvm.modf.ppcf128(ppc_fp128 %{{.+}})
+  // X86F128: call { fp128, fp128 } @llvm.modf.f128(fp128 %{{.+}})
   // PPCF128: call fp128 @modff128(fp128 noundef %{{.+}}, ptr noundef %{{.+}})
   __builtin_modfl(f,l);
 
diff --git a/clang/test/CodeGen/math-libcalls.c b/clang/test/CodeGen/math-libcalls.c
index 14fdee77f4d78..ad297828f48ed 100644
--- a/clang/test/CodeGen/math-libcalls.c
+++ b/clang/test/CodeGen/math-libcalls.c
@@ -83,12 +83,12 @@ void foo(double *d, float f, float *fp, long double *l, int *i, const char *c) {
 
   modf(f,d);       modff(f,fp);      modfl(f,l);
 
-  // NO__ERRNO: declare double @modf(double noundef, ptr noundef) [[NOT_READNONE]]
-  // NO__ERRNO: declare float @modff(float noundef, ptr noundef) [[NOT_READNONE]]
-  // NO__ERRNO: declare x86_fp80 @modfl(x86_fp80 noundef, ptr noundef) [[NOT_READNONE]]
-  // HAS_ERRNO: declare double @modf(double noundef, ptr noundef) [[NOT_READNONE]]
-  // HAS_ERRNO: declare float @modff(float noundef, ptr noundef) [[NOT_READNONE]]
-  // HAS_ERRNO: declare x86_fp80 @modfl(x86_fp80 noundef, ptr noundef) [[NOT_READNONE]]
+  // NO__ERRNO: declare { double, double } @llvm.modf.f64(double) [[READNONE_INTRINSIC]]
+  // NO__ERRNO: declare { float, float } @llvm.modf.f32(float) [[READNONE_INTRINSIC]]
+  // NO__ERRNO: declare { x86_fp80, x86_fp80 } @llvm.modf.f80(x86_fp80) [[READNONE_INTRINSIC]]
+  // HAS_ERRNO: declare { double, double } @llvm.modf.f64(double) [[READNONE_INTRINSIC]]
+  // HAS_ERRNO: declare { float, float } @llvm.modf.f32(float) [[READNONE_INTRINSIC]]
+  // HAS_ERRNO: declare { x86_fp80, x86_fp80 } @llvm.modf.f80(x86_fp80) [[READNONE_INTRINSIC]]
   // HAS_MAYTRAP: declare double @modf(double noundef, ptr noundef) [[NOT_READNONE]]
   // HAS_MAYTRAP: declare float @modff(float noundef, ptr noundef) [[NOT_READNONE]]
   // HAS_MAYTRAP: declare x86_fp80 @modfl(x86_fp80 noundef, ptr noundef) [[NOT_READNONE]]

MacDue · 2025-03-06T09:43:51Z

Note: Changes are the same as the original patch.

zmodem · 2025-03-10T15:35:32Z

This broke code using modff on 32-bit x86 Windows, where the runtime library doesn't provide any modff symbol, rather it's implemented inline in math.h (which calls modf):

>type \src\temp\a.c
#include <math.h>
#include <stdio.h>

float foo(float f) {
  float i;
  return modff(f, &i);
}

int main() {
        printf("%f\n", foo(3.14));
        return 0;
}

>build\bin\clang-cl -m32 \src\temp\a.c
a-4184ec.obj : error LNK2019: unresolved external symbol _modff referenced in function _foo
a.exe : fatal error LNK1120: 1 unresolved externals
clang-cl: error: linker command failed with exit code 1120 (use -v to see invocation)

(See also https://godbolt.org/z/jvfe4zGTj)

I'll back this out for now to unbreak things.

…c" (#129885)" This broke modff calls on 32-bit x86 Windows. See comment on the PR. > This updates the existing modf[f|l] builtin to be lowered via the > llvm.modf.* intrinsic (rather than directly to a library call). > > The legalization issues exposed by the original PR (#126750) should have > been fixed in #128055 and #129264. This reverts commit cd1d9a8.

nico · 2025-03-10T15:39:46Z

Here are a few ideas for possible paths forward: https://issues.chromium.org/issues/401571943#comment9

…actually, let me inline them:

if (Builder.getIsFPConstrained()) -> if (Builder.getIsFPConstrained() && !isTargetingWin32())?

Alternatively, we could put an actual symbol in compiler-rt that calls modff from the CRT, which puts an available symbol into compiler-rt. (…can compiler-rt depend on CRT?)

There's probably nothing that guarantees that the symbol isn't inline on other platforms either. …but we do have the inline symbol. Maybe LLVM doesn't know to mark the inline as used when it's called through the intrinsic? That could be a third idea to look into.

MacDue · 2025-03-10T16:27:05Z

Thanks for the report 👍 It looks like all we have to do is set the legalization rule for f32 to "Promote". Looks like there's actually already logic to do this for a bunch of other nodes:

From X86ISelLowering.cpp:

  // On 32 bit MSVC, `fmodf(f32)` is not defined - only `fmod(f64)`
  // is. We should promote the value to 64-bits to solve this.
  // This is what the CRT headers do - `fmodf` is an inline header
  // function casting to f64 and calling `fmod`.
  if (Subtarget.is32Bit() &&
      (Subtarget.isTargetWindowsMSVC() || Subtarget.isTargetWindowsItanium()))
    // clang-format off
   for (ISD::NodeType Op :
         {ISD::FACOS,  ISD::STRICT_FACOS,
          ISD::FASIN,  ISD::STRICT_FASIN,
          ISD::FATAN,  ISD::STRICT_FATAN,
          ISD::FATAN2, ISD::STRICT_FATAN2,
          ISD::FCEIL,  ISD::STRICT_FCEIL,
          ISD::FCOS,   ISD::STRICT_FCOS,
          ISD::FCOSH,  ISD::STRICT_FCOSH,
          ISD::FEXP,   ISD::STRICT_FEXP,
          ISD::FFLOOR, ISD::STRICT_FFLOOR,
          ISD::FREM,   ISD::STRICT_FREM,
          ISD::FLOG,   ISD::STRICT_FLOG,
          ISD::FLOG10, ISD::STRICT_FLOG10,
          ISD::FPOW,   ISD::STRICT_FPOW,
          ISD::FSIN,   ISD::STRICT_FSIN,
          ISD::FSINH,  ISD::STRICT_FSINH,
          ISD::FTAN,   ISD::STRICT_FTAN,
          ISD::FTANH,  ISD::STRICT_FTANH})
      if (isOperationExpand(Op, MVT::f32))
        setOperationAction(Op, MVT::f32, Promote);
  // clang-format on

…#129885) Reverts llvm#127987 Original description: This updates the existing modf[f|l] builtin to be lowered via the llvm.modf.* intrinsic (rather than directly to a library call). The legalization issues exposed by the original PR (llvm#126750) should have been fixed in llvm#128055 and llvm#129264.

Revert "Revert "[clang] Lower modf builtin using llvm.modf intrinsi…

f414ee4

…c" (#127…" This reverts commit d595fc9.

llvmbot added clang Clang issues not falling into any other category backend:X86 clang:codegen IR generation bugs: mangling, exceptions, etc. labels Mar 5, 2025

MacDue added the skip-precommit-approval PR for CI feedback, not intended for review label Mar 6, 2025

MacDue merged commit cd1d9a8 into main Mar 6, 2025
16 checks passed

MacDue deleted the revert-127987-revert-126750-modf_builtin branch March 6, 2025 09:44

MacDue mentioned this pull request Mar 10, 2025

[SDAG][X86] Promote float FMODF to double on 32-bit Windows #130636

Merged

shiltian mentioned this pull request Mar 10, 2025

[AMDGPU] Fix test failures when expensive checks are enabled #130644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reland "[clang] Lower modf builtin using `llvm.modf` intrinsic" #129885

Reland "[clang] Lower modf builtin using `llvm.modf` intrinsic" #129885

MacDue commented Mar 5, 2025

llvmbot commented Mar 5, 2025 •

edited

Loading

MacDue commented Mar 6, 2025

zmodem commented Mar 10, 2025

nico commented Mar 10, 2025

MacDue commented Mar 10, 2025

Reland "[clang] Lower modf builtin using llvm.modf intrinsic" #129885

Reland "[clang] Lower modf builtin using llvm.modf intrinsic" #129885

Conversation

MacDue commented Mar 5, 2025

llvmbot commented Mar 5, 2025 • edited Loading

MacDue commented Mar 6, 2025

zmodem commented Mar 10, 2025

nico commented Mar 10, 2025

MacDue commented Mar 10, 2025

Reland "[clang] Lower modf builtin using `llvm.modf` intrinsic" #129885

Reland "[clang] Lower modf builtin using `llvm.modf` intrinsic" #129885

llvmbot commented Mar 5, 2025 •

edited

Loading