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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
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() &&
----------------
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.


================
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);
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7212
+
+    if (RedCost.isValid() && RedCost < ExtCost * 2 + MulCost + BaseCost)
+      return I == RetI ? RedCost : 0;
----------------
SjoerdMeijer wrote:
> Just a quick query on the  `* 2`, was wondering if that needs to be 3, but probably depends on my earlier question about matching op1.
It could be either way, I think, but the third extend would be of a different type. Because they are equivalent (https://alive2.llvm.org/ce/z/1dVe_y), I was just taking the simpler route and costing them as two larger extends and a multiply.

I have improved that to use the original types though. It will be good for it to be more accurate.


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

https://reviews.llvm.org/D106166