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

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau 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())
----------------
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.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113095