[PATCH] D111555: [LoopVectorize] Add strict reduction support for fmuladd intrinsic

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

Just a flyby review triggered by commenting on D111630 <https://reviews.llvm.org/D111630> so my comments are more stylistic in nature rather than digging into the technical details.



================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:269-270
+  static bool isFMulAddIntrinsic(Instruction *I) {
+    return match(
+        I, m_Intrinsic<Intrinsic::fmuladd>(m_Value(), m_Value(), m_Value()));
+  }
----------------
I feel like `isa<IntrinsicInst>(I) && cast<IntrinsicInst>(I)->getIntrinsicID() == Intrinsic::fmuladd` would be a cheaper way to get the same result?


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:205-210
+  // Recognise a call to the llvm.fmuladd intrinsic.
+  bool IsFMulAdd = RecurrenceDescriptor::isFMulAddIntrinsic(Exit);
+
+  if ((Exit->getOpcode() != Instruction::FAdd && !IsFMulAdd) ||
+      Exit != ExactFPMathInst)
     return false;
----------------
Just a suggestion but given this function no longer has a single instruction to care about perhaps it's worth being more explicit.  For example:
```
if (Kind == RecurKind::FAdd && Exit->getOpcode() != Instruction::FAdd)
  return false;
if (Kind == RecurKind::FMulAdd && RecurrenceDescriptor::isFMulAddIntrinsic(Exit))
  return false
if (Exit != ExactFPMathInst)
  return false;
```


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:216-218
+  if ((!IsFMulAdd && LHS != Phi && RHS != Phi) ||
+      (IsFMulAdd && Exit->getOperand(2) != Phi))
     return false;
----------------
Much like my previous suggestion what about:
```
if (Kind == RecurKind::FAdd && Op0 != Phi && Op1 != Phi)
  return false;
if (Kind == RecurKind::FMulAdd && Exit->getOperand(2) != Phi)
  return false;
```


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9796
+        FMF = FPMO->getFastMathFlags();
+        State.Builder.setFastMathFlags(FMF);
+      }
----------------
I'm assuming this change will remain live after this point and thus affect nodes created after exiting this function? Which would be bad.  In any case I think that instead of calling `CreateBinOp` you can instead call `CreateFMulFMF` which will propagate the FMF flags for you.


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

https://reviews.llvm.org/D111555