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

[Peter Waller via Phabricator via llvm-commits]

peterwaller-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())
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113095