This is an archive of the discontinued LLVM Phabricator instance.

[flang] Add fastmath flags to localBuilder in IntrinsicCall
ClosedPublic

Authored by DavidTruby on Jul 6 2023, 7:39 AM.

Details

Summary

Currently the local builder used in IntrinsicCall doesn't have the
fastmath flags passed to it. This results in the fastmath attribute
not being added to certain runtime calls. This patch simply forwards
the fastmath flags from the parent builder.

Diff Detail

Event Timeline

DavidTruby created this revision.Jul 6 2023, 7:39 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 6 2023, 7:39 AM
DavidTruby requested review of this revision.Jul 6 2023, 7:39 AM

The main motivation of this is so that we can access the fastmath flags when generating complex operations, and only use the mlir complex dialect when approx-func is enabled. I have another patch ready for that but wanted to post this separately to make the churn a bit more clear.
I think these operations should have the fastmath attribute anyway though, if I understand correctly? Was there a specific reason that they didn't?

vzakhari added inline comments.Jul 6 2023, 11:08 AM
flang/lib/Optimizer/Builder/IntrinsicCall.cpp
1628

This might be okay until we decide to support different FastMathFlags settings for different call sites in the same compilation module (e.g. via some compiler directive).

Would you mind mangling the wrapper name with getFastMathFlagsString(builder) value (e.g. as in SimplifyIntrinsics.cpp)? This should help avoiding any future conflicts between the call sites.

Mangle outlined function names to reflect fastmath flags

LGTM in general. Please apply clang-format.

flang/lib/Optimizer/Builder/IntrinsicCall.cpp
1697

nit: I would prefer using . here, because it makes the base name (e.g. cos, exp, etc.) separated from the FMF string in a more explicit fashion. In intrinsics simplification this separation is more obvious due to us adding x... rank specification to the name.

Feel free to keep it as-is, since this is just my preference.

DavidTruby added inline comments.Jul 10 2023, 2:53 PM
flang/lib/Optimizer/Builder/IntrinsicCall.cpp
1697

I also thought . might look better, but I copied the _ convention from SimplifyIntrinsic.cpp as I wanted to be consistent. Happy to change this to . if _ is used there for a different reason I didn't understand.

vzakhari added inline comments.Jul 10 2023, 3:18 PM
flang/lib/Optimizer/Builder/IntrinsicCall.cpp
1697

Functions created in SimplifyIntrinsics are intentionally linkonce-odr external symbols, so I wanted to use C-friendly names for them. The intrinsics wrappers are supposed to be purely internal to the compilation module and we can use whatever names for them. Since . has been used for them, I think we can stick to it.

Change _ to . in name mangling and run clang-format

vzakhari accepted this revision.Jul 10 2023, 9:20 PM

Thank you!

This revision is now accepted and ready to land.Jul 10 2023, 9:20 PM

I appear to have not taken PPC into account here: https://lab.llvm.org/buildbot/#/builders/21/builds/74994
Will push a fix asap