[PATCH] D113095: Combine FADD and FMUL aarch64 intrinsics to FMLA

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:713-714
+  llvm::FastMathFlags FAddFlags = II.getFastMathFlags();
+  // Don't combine when FMul & FAdd flags differ to prevent the loss of any
+  // additional important flags
+  if (FAddFlags != cast<CallInst>(FMul)->getFastMathFlags())
----------------
MattDevereau wrote:
> peterwaller-arm wrote:
> > I think this comment needs work. The bar needs to be: if someone who is not us reads this in the future, does it help them understand what is going on? For example, the qualifier 'important' -- it is going to be difficult for a future reader to arrive at the same understanding as you have about what an 'important flag' is. Important for what? The set of flags in existence may be different in the future, for example. I've suggested an edit, please only accept if it makes sense to you and feel free to refine it further.
> I'm not a fan of using language that refers to possible future work, i.e. "For the time being", if you want the reader to understand what's going on here with this comment then referring to work that might not even happen instead of whats literally happening seems a bit strange to me. Also I don't really like using less common synonyms like inhibit or elide. what about:
> 
> ```
> // FMLA must merge two sets of flags into one. Stop the combine if the flags on the inputs differ in case
> // they allow other optimizations to skip the operation entirely.
> ```
For what it's worth I think I agree with @MattDevereau. To be honest, I thought the original comment seemed fine to me. Perhaps it's worth emphasising that combining the operations requires taking the common subset of flags and potentially dropping ones that would otherwise permit more powerful optimisations to take place? i.e. something like

  // FMLA must merge two sets of flags into one, taking only the common subset.
  // Stop the combine when the flags on the inputs differ in case dropping flags would lead to
  // us missing out on more beneficial optimizations.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113095