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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:701
+  // fold (fadd p a (fmul p b c)) -> (fma p a b c)
+  Value *p = II.getOperand(0);
+  Value *a = II.getOperand(1);
----------------
These should be upper case to match LLVM styling and be consistent with the other names using within this patch.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:706
+  if (!match(FMul, m_Intrinsic<Intrinsic::aarch64_sve_fmul>(
+                       m_Deferred(p), m_Value(b), m_Value(c))))
+    return None;
----------------
This can be `m_Specific` as there is nothing to defer.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:718
+    return None;
+  if (!FAddFlags.allowContract() || !FMulFlags.allowContract())
+    return None;
----------------
Given by this point you've proven `FAddFlags == FMulFlags` you only need to check `FAddFlags` for `allowContract` which means you don't really need the `FMulFlags` temporary as they'll only be a single use remaining.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:760-762
+  auto FMLA = instCombineSVEVectorFMLA(IC, II);
+  if (FMLA)
+    return FMLA;
----------------
I think the following is more in keeping with LLVM styling.
```
if (auto FMLA = instCombineSVEVectorFMLA(IC, II))
  return FMLA;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113095