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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
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. |
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. |
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. |
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. |
I appear to have not taken PPC into account here: https://lab.llvm.org/buildbot/#/builders/21/builds/74994
Will push a fix asap
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.