Skip to content

[flang] Implement sinpi #149525

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 23, 2025
Merged

[flang] Implement sinpi #149525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions flang/include/flang/Optimizer/Builder/IntrinsicCall.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,7 @@ struct IntrinsicLibrary {
mlir::Value genShiftA(mlir::Type resultType, llvm::ArrayRef<mlir::Value>);
mlir::Value genSign(mlir::Type, llvm::ArrayRef<mlir::Value>);
mlir::Value genSind(mlir::Type, llvm::ArrayRef<mlir::Value>);
mlir::Value genSinpi(mlir::Type, llvm::ArrayRef<mlir::Value>);
fir::ExtendedValue genSize(mlir::Type, llvm::ArrayRef<fir::ExtendedValue>);
fir::ExtendedValue genSizeOf(mlir::Type, llvm::ArrayRef<fir::ExtendedValue>);
mlir::Value genSpacing(mlir::Type resultType,
Expand Down
1 change: 1 addition & 0 deletions flang/lib/Evaluate/intrinsics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -957,6 +957,7 @@ static const IntrinsicInterface genericIntrinsicFunction[]{
{"sin", {{"x", SameFloating}}, SameFloating},
{"sind", {{"x", SameFloating}}, SameFloating},
{"sinh", {{"x", SameFloating}}, SameFloating},
{"sinpi", {{"x", SameFloating}}, SameFloating},
{"size",
{{"array", AnyData, Rank::arrayOrAssumedRank},
OptionalDIM, // unless array is assumed-size
Expand Down
16 changes: 16 additions & 0 deletions flang/lib/Optimizer/Builder/IntrinsicCall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -901,6 +901,7 @@ static constexpr IntrinsicHandler handlers[]{
{{{"number", asValue}, {"handler", asAddr}, {"status", asAddr}}},
/*isElemental=*/false},
{"sind", &I::genSind},
{"sinpi", &I::genSinpi},
{"size",
&I::genSize,
{{{"array", asBox},
Expand Down Expand Up @@ -8060,6 +8061,21 @@ mlir::Value IntrinsicLibrary::genSind(mlir::Type resultType,
return getRuntimeCallGenerator("sin", ftype)(builder, loc, {arg});
}

// SINPI
mlir::Value IntrinsicLibrary::genSinpi(mlir::Type resultType,
llvm::ArrayRef<mlir::Value> args) {
assert(args.size() == 1);
mlir::MLIRContext *context = builder.getContext();
mlir::FunctionType ftype =
mlir::FunctionType::get(context, {resultType}, {args[0].getType()});
llvm::APFloat pi = llvm::APFloat(llvm::numbers::pi);
mlir::Value dfactor =
builder.createRealConstant(loc, mlir::Float64Type::get(context), pi);
mlir::Value factor = builder.createConvert(loc, args[0].getType(), dfactor);
Comment on lines +8072 to +8074
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not create the real constant with the argument type in the first place? This might matter for real(16).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mlir::Value factor = builder.createRealConstant(loc, args[0].getType(), pi);

Using the argument type in the creation process seems to result in the following error:

error: FloatAttr type doesn't match the type implied by its value
flang: /home/c8ef/llvm-dev/mlir/include/mlir/IR/StorageUniquerSupport.h:180: static ConcreteT mlir::detail::StorageUserBase<mlir::FloatAttr, mlir::Attribute, mlir::detail::FloatAttrStorage, mlir::detail::AttributeUniquer, mlir::TypedAttr::Trait>::get(MLIRContext *, Args &&...) [ConcreteT = mlir::FloatAttr, BaseT = mlir::Attribute, StorageT = mlir::detail::FloatAttrStorage, UniquerT = mlir::detail::AttributeUniquer, Traits = <mlir::TypedAttr::Trait>, Args = <mlir::Type &, const llvm::APFloat &>]: Assertion `succeeded( ConcreteT::verifyInvariants(getDefaultDiagnosticEmitFn(ctx), args...))' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: /home/c8ef/llvm-dev/build/bin/flang -fc1 -emit-fir /home/c8ef/llvm-dev/flang/test/Lower/Intrinsics/sinpi.f90 -o -
 #0 0x0000ffffa6de9e50 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/c8ef/llvm-dev/llvm/lib/Support/Unix/Signals.inc:834:11
 #1 0x0000ffffa6dea368 PrintStackTraceSignalHandler(void*) /home/c8ef/llvm-dev/llvm/lib/Support/Unix/Signals.inc:918:1
 #2 0x0000ffffa6de83f4 llvm::sys::RunSignalHandlers() /home/c8ef/llvm-dev/llvm/lib/Support/Signals.cpp:104:5
 #3 0x0000ffffa6deab18 SignalHandler(int, siginfo_t*, void*) /home/c8ef/llvm-dev/llvm/lib/Support/Unix/Signals.inc:426:38
 #4 0x0000ffffaa031808 (linux-vdso.so.1+0x808)
 #5 0x0000ffffa64d7dc0 __pthread_kill_implementation ./nptl/pthread_kill.c:44:76
 #6 0x0000ffffa6486980 raise ./signal/../sysdeps/posix/raise.c:27:6
 #7 0x0000ffffa6471ac4 abort ./stdlib/abort.c:81:3
 #8 0x0000ffffa647f9bc __assert_fail_base ./assert/assert.c:53:15
 #9 0x0000ffff9fc572cc mlir::FloatAttr mlir::detail::StorageUserBase<mlir::FloatAttr, mlir::Attribute, mlir::detail::FloatAttrStorage, mlir::detail::AttributeUniquer, mlir::TypedAttr::Trait>::get<mlir::Type&, llvm::APFloat const&>(mlir::MLIRContext*, mlir::Type&, llvm::APFloat const&) /home/c8ef/llvm-dev/mlir/include/mlir/IR/StorageUniquerSupport.h:179:5
#10 0x0000ffff9fc4c8c8 mlir::FloatAttr::get(mlir::Type, llvm::APFloat const&) /home/c8ef/llvm-dev/build/tools/mlir/include/mlir/IR/BuiltinAttributes.cpp.inc:293:10
#11 0x0000ffff9fc3b3bc mlir::Builder::getFloatAttr(mlir::Type, llvm::APFloat const&) /home/c8ef/llvm-dev/mlir/lib/IR/Builders.cpp:254:10
#12 0x0000ffff944e0b84 fir::FirOpBuilder::createRealConstant(mlir::Location, mlir::Type, llvm::APFloat const&) /home/c8ef/llvm-dev/flang/lib/Optimizer/Builder/FIRBuilder.cpp:187:17
#13 0x0000ffff94567eac fir::IntrinsicLibrary::genSinpi(mlir::Type, llvm::ArrayRef<mlir::Value>) /home/c8ef/llvm-dev/flang/lib/Optimizer/Builder/IntrinsicCall.cpp:8072:32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LogicalResult FloatAttr::verify(function_ref<InFlightDiagnostic()> emitError,
                                Type type, APFloat value) {
  // Verify that the type is correct.
  if (!llvm::isa<FloatType>(type))
    return emitError() << "expected floating point type";

  // Verify that the type semantics match that of the value.
  if (&llvm::cast<FloatType>(type).getFloatSemantics() !=
      &value.getSemantics()) {
    return emitError()
           << "FloatAttr type doesn't match the type implied by its value";
  }
  return success();
}

It appears these two floating-point types have different semantics. To ensure pi uses the same semantics as the argument type, we'd still need to convert it from APFloat. Either way, a conversion seems necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh okay what I think is happening here is that APFloat is forcing its size to f64 because llvm::numbers::pi is defined as a double. I'm concerned that this could produce inaccurate results for real(16) (aka floating point types with more precision than f64: f128 on aarch64 systems, I think f80 on x86).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah so I think the ultimate solution for trig-pi functions is to use the glibc version, when we have glibc>=2.41(it does provide f128 version). And if not maybe we can use this as a fallback or something like libquadmath. Or maybe we need a interface return pi based on fltSemantic. Not quite sure how to do it right since there are a lot of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, handling these is somewhat tricky. I'm not exactly sure how to handle this correctly in flang. I am currently working on adding sinpi-related functions to libquadmath in the GCC source tree. The GCC maintainers are not satisfied with the precision of the generic implementation imported from glibc, so this patch has not been merged yet.

C23 has added support for sinpi-related functions, and eventually libc will also have support. This will allow us to achieve better precision. If neither glibc nor libquadmath have it, we can fallback to our current approach.

However, glibc does not provide sind-related functions. We just need to fix the constant pi type later.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you may follow the implementation of SinF128 as Tom suggested. As long as libquadmath does not support sinpi for float128 you may implement SinpiF128 in flang-rt/lib/quadmath same way you implement it here but using proper PI definition from https://gcc.gnu.org/onlinedocs/libquadmath/Typedef-and-constants.html

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 think this is a good approach for handling the f128 case. However, if we implement this logic in libquadmath, will it diverge from how we handle f32/f64 here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Diverge in what sense?

If we "lower" the f32/f64 cases here as sin(PI * x), and the f128 case in Flang's quadmath as sinq(M_PIq * x), then they should be pretty consistent, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right. I get it. That seems like a good approach.

mlir::Value arg = builder.create<mlir::arith::MulFOp>(loc, args[0], factor);
return getRuntimeCallGenerator("sin", ftype)(builder, loc, {arg});
}

// SIZE
fir::ExtendedValue
IntrinsicLibrary::genSize(mlir::Type resultType,
Expand Down
22 changes: 22 additions & 0 deletions flang/test/Lower/Intrinsics/sinpi.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
! RUN: %flang_fc1 -emit-fir %s -o - | FileCheck %s --check-prefixes="CHECK"

function test_real4(x)
real :: x, test_real4
test_real4 = sinpi(x)
end function

! CHECK-LABEL: @_QPtest_real4
! CHECK: %[[dfactor:.*]] = arith.constant 3.1415926535897931 : f64
! CHECK: %[[factor:.*]] = fir.convert %[[dfactor]] : (f64) -> f32
! CHECK: %[[mul:.*]] = arith.mulf %{{.*}}, %[[factor]] fastmath<contract> : f32
! CHECK: %[[sin:.*]] = math.sin %[[mul]] fastmath<contract> : f32

function test_real8(x)
real(8) :: x, test_real8
test_real8 = sinpi(x)
end function

! CHECK-LABEL: @_QPtest_real8
! CHECK: %[[dfactor:.*]] = arith.constant 3.1415926535897931 : f64
! CHECK: %[[mul:.*]] = arith.mulf %{{.*}}, %[[dfactor]] fastmath<contract> : f64
! CHECK: %[[sin:.*]] = math.sin %[[mul]] fastmath<contract> : f64
Loading