[PATCH] D111638: [AArch64][SVE] Combine predicated FMUL/FADD into FMA

[Bradley Smith via Phabricator via llvm-commits]

bsmith added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:719-723
+  llvm::FastMathFlags FMulFlags = cast<IntrinsicInst>(FMul)->getFastMathFlags();
+  if (!FAddFlags.allowContract() || !FMulFlags.allowContract())
+    return None;
+  if (FAddFlags != FMulFlags)
+    return None;
----------------
None of this seems to take into account the global fast-math options, i.e. the "unsafe-fp-math"="true" attribute, hence I don't think this optimization can ever be triggered from C, only directly written IR with the fast flags.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:720
+  llvm::FastMathFlags FMulFlags = cast<IntrinsicInst>(FMul)->getFastMathFlags();
+  if (!FAddFlags.allowContract() || !FMulFlags.allowContract())
+    return None;
----------------
Do we not care about Reassociation here also?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:722-723
+    return None;
+  if (FAddFlags != FMulFlags)
+    return None;
+
----------------
I feel like it might just be sufficient check to check both operands for reassociation and contraction, rather than a full flag check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111638