[PATCH] D77074: [FPEnv][AArch64] Platform-specific builtin constrained FP enablement

[Kevin P. Neal via Phabricator via cfe-commits]

kpn marked an inline comment as done.
kpn added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:8486-8492
+      return Builder.CreateConstrainedFPCall(
+          F,
+          {EmitScalarExpr(E->getArg(1)), EmitScalarExpr(E->getArg(2)), Ops[0]});
+    } else {
+      Function *F = CGM.getIntrinsic(Intrinsic::fma, HalfTy);
+      // NEON intrinsic puts accumulator first, unlike the LLVM fma.
+      return Builder.CreateCall(F, {EmitScalarExpr(E->getArg(1)),
----------------
dnsampaio wrote:
> It seems that `Builder.CreateCall` and `Builder.CreateConstrainedFPCall` usually have the same arguments, except for the function F being or not part of "experimental_constrained_".
> Would it make sense to teach the `Builder` to select between creating a constrained or not call, depending if the function passed is constrained?
> 
> I was thinking in something like this:
> ```
> Function *F = CGM.getIntrinsic( Builder.getIsFPConstrained()?
>                                                         Intrinsic::experimental_constrained_fma :
>                                                         Intrinsic::fma, HalfTy);
> return Builder.CreateCallConstrainedFPIfRequired(F, ....
> ```
> 
In CGBuiltins.cpp we already have emitUnaryMaybeConstrainedFPBuiltin() plus Binary and Ternary. They work well for the pattern seen on other hosts. But they won't easily work for this ticket. 

How about a new function just below those three that will work well here? A emitCallMaybeConstrainedFPBuiltin() that takes two intrinsic IDs and chooses which one based on constrained FP would make for an even more compact use. The block of example code you put above would just turn into a single function call. Does that work for you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77074