[PATCH] D69798: Implement inlining of strictfp functions

[Simon Moll via Phabricator via llvm-commits]

simoll added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CloneFunction.cpp:310
+  Instruction *NewInst;
+  if (HostFuncIsStrictFP && OldInst.dependsOnFPEnvironment()) {
+    // Instead of cloning the instruction, a call to constrained intrinsic
----------------
sepavloff wrote:
> simoll wrote:
> > andrew.w.kaylor wrote:
> > > There is some ongoing discussion about how predicated vector instructions will handle constrained FP mode. I think Simon's intention is to have just a single intrinsic that is used regardless of whether strictfp semantics are needed.
> > > 
> > > Also, in addition to converting fp intrinsics to strict equivalents, I think we need to add the strictfp attribute to callsites. We're currently preventing libcall simplification for callsites marked strictfp. The plan has been for front ends to mark all calls as strictfp so that they don't need to identify math library calls. I could probably be convinced that this is not necessary. Arguably, simplifyLibCalls could look at the callee's function attributes instead. @kpn has been considering whether this behavior should be relaxed.
> > Yes, the idea for [LLVM-VP](https://reviews.llvm.org/D57504) is to have only one set of fp intrinsics for both constrained and default-env fp ops (eg `llvm.vp.fadd`).
> > The intrinsic declarations are qualified as unconstrained by default (the //function// has the `readnone` attribute and does not have `strictfp`).
> > If the VP call does not have the `strictfp` attribute, the metadata params have to specify the default-fp env.
> > 
> > The VP fp intrinsics are constrained, only if the //callsite// has the `strictfp` attribute. In that case, the same rules as for `llvm.experimental.constrained.*` intrinsics apply.
> > 
> > Yes, the idea for LLVM-VP is to have only one set of fp intrinsics for both constrained and default-env fp ops (eg llvm.vp.fadd).
> 
> It means that these calls do not need to be transformed and they may be processes as any other intrinsic.
> 
> > The VP fp intrinsics are constrained, only if the callsite has the strictfp attribute.
> 
> As @andrew.w.kaylor pointed out, all function calls need to have `strictfp` attribute, it is now attached to all calls in the inlined function.
> As @andrew.w.kaylor pointed out, all function calls need to have strictfp attribute, it is now attached to all calls in the inlined function.
Great! That should work seamlessly for VP intrinsics once we enable their fp-constrained usage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69798