[PATCH] D132477: Improve cost model for some 128-bit vector operations that use SVE

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2068
+        // consider the cost of legalizaiton
+        Cost *= LT.first;
+      } else {
----------------
hassnaa-arm wrote:
> david-arm wrote:
> > I am so sorry about this @hassnaa-arm, but I made a mistake previously asking you to multiply by the legalisation cost. It seems that `BaseT::getArithmeticInstrCost` already takes this into account.
> > 
> > Also, I didn't spot this before, but I think we should actually return the cost here, i.e.
> > 
> >   return Cost;
> > 
> > because otherwise we fall through to code below that doubles the cost again, i.e.
> > 
> >   Cost += Cost;
> so what is the problem of doubling the cost ? I understand that because there are 2 operands, and the same work should be done for both of them, and the code calculates the cost of one of them, so, we have to double the cost to consider the needed work for both operands.
So from looking at the comments

      // TODO: if one of the arguments is scalar, then it's not necessary to
      // double the cost of handling the vector elements.

I think the doubling was added specifically for when we are scalarising the operation, i.e. extracting the vector elements of each operand, performing a scalar multiply, then inserting the result back into the final vector. So the doubling here refers to the scalarisation of both operands, not just one. However, when using SVE we are not scalarising each operand so we don't need to double the cost. To be honest, I don't think the existing code is quite right either because I think it should really be (BaseCost + (2 * InsertExtractCost)), but I don't think we should change it in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132477