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

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added a comment.

Wrong argument order needs fixing, and some nits. Please also run clang-format.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:700
+                                                        IntrinsicInst &II) {
+  // fold (fadd a (fmul b c)) -> (fma a b c)
+  Value *p, *FMul, *a, *b, *c;
----------------
The fold as written here looks correct to me, but the match in m_SVEFAdd isn't a 1:1 with this (it is `(fadd (fmul a b) c)`, which is different). The effect is that the resulting fma from the combine has the wrong argument order.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:714
+                            m_Value(c))))
+    return None;
+  auto FMulInst = dyn_cast<IntrinsicInst>(FMul);
----------------
Please put a line break after this.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:715
+    return None;
+  auto FMulInst = dyn_cast<IntrinsicInst>(FMul);
+  if (!FMulInst->hasOneUse() || (p != II.getOperand(0)))
----------------
Is the dyn_cast needed? You should only need it if you need some methods which are on IntrinsicInst but not Value.

Also, nit, dyn_cast has logic which checks the type of the thing being cast -- this is unnecessary because the type is already constrained by the match() logic above, so if you did need a cast you can write `cast<Ty>(Val)` instead of `dyn_cast`.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:716
+  auto FMulInst = dyn_cast<IntrinsicInst>(FMul);
+  if (!FMulInst->hasOneUse() || (p != II.getOperand(0)))
+    return None;
----------------
It seems to me that p != II.getOperand(0) should already be being checked by m_Deferred?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:720
+  llvm::FastMathFlags FMulFlags = FMulInst->getFastMathFlags();
+  if(!(FAddFlags.allowContract() && FMulFlags.allowContract()) || FAddFlags != FMulFlags)
+    return None;
----------------
Nit. I think this condition would read a bit more clearly if it were split into two, and negate the contraction allow.
```
if (!FAddFlags.allowContract() || !FMulFlags.allowContract())
  return false;
```

And the second condition is a bit hidden on the right compared to just having it as a separate `if (FAddFlags != FMulFlags) return None`.




================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:762
+                                                        IntrinsicInst &II) {
+  auto FMLA = instCombineSVEVectorFmla(IC, II);
+  if (FMLA)
----------------
Nit. `FMLA` and `Fmla` (in the function name). I think it should be consistent on `FMLA`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111638