[PATCH] D106166: [LV][ARM] Tighten up MLA reduction costing

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Cheers, looks like a good change to me.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7193
+            m_ZExtOrSExt(m_Mul(m_Instruction(Op0), m_Instruction(Op1)))) &&
+      match(Op0, m_ZExtOrSExt(m_Value())) &&
+      Op0->getOpcode() == Op1->getOpcode() &&
----------------
dmgreen wrote:
> SjoerdMeijer wrote:
> > Do we also need to match `op1`?
> > 
> >   match(Op1, m_ZExtOrSExt(m_Value())
> > 
> > that's what I would guess from reading the comment below.
> We check Op0->getOpcode() == Op1->getOpcode(), so do this without a matcher.
Ah, okay, missed it, but there it is.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7198
+      (Op0->getOpcode() == RedOp->getOpcode() || Op0 == Op1)) {
+    // Matched reduce(ext(mul(ext(A), ext(B)))
+    bool IsUnsigned = isa<ZExtInst>(Op0);
----------------
dmgreen wrote:
> SjoerdMeijer wrote:
> > Nit: might be easier to read if this comment is just before the if. 
> The comments are all inside the `ifs` to be consistent with the other code here. Otherwise they need to be before the `else`, and I'm not sure that is very readable.
Okay, not a big fan of that style, but fair enough.


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

https://reviews.llvm.org/D106166