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

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added a comment.

Thanks for your review @bsmith, a couple of thoughts on your queries.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:720
+  llvm::FastMathFlags FMulFlags = cast<IntrinsicInst>(FMul)->getFastMathFlags();
+  if (!FAddFlags.allowContract() || !FMulFlags.allowContract())
+    return None;
----------------
bsmith wrote:
> Do we not care about Reassociation here also?
My read of the LangRef suggests that contract allows an FMA contraction (But not two of them). So to my eyes this looks sufficient. Please can you clarify your concern?


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

The full check was chosen because:
* We're taking two instructions and turning them into one.
* Therefore we have two sets of flags.
* We need to put some flags on the resulting op.
* We could intersect them, but that would result in losing flags.
* It's conceivable that the lost flags could allow for the whole op to be removed by dead code elimination.
  * (I don't have a concrete example of this in practice, but maybe via nnan/ninf for example?)
* Therefore, the conservative thing to do is only to apply the optimization if the flags are equal (and to preserve the flags).
* This hasn't been a data-driven analysis but it seems a conservative one to work with for the moment. If there is evidence to consider an alternative approach, we'd consider it.

The DAG level FMA contractions don't bother with this, but they are running late in the pipeline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111638