[PATCH] D103952: [CostModel][AArch64] Improve the cost estimate of CTPOP intrinsic

[Rosie Sumpter via Phabricator via llvm-commits]

RosieSumpter added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:289
+    MVT MTy = MVT::getIntegerVT(RetTy->getScalarSizeInBits());
+    if (const auto *Entry = CostTableLookup(CtpopCostTbl, ISD::CTPOP, MTy)) {
+      return LT.first * Entry->Cost;
----------------
SjoerdMeijer wrote:
> SjoerdMeijer wrote:
> > Nit: you don't need the curly brackets for an if with 1 statement.
> Do we have a test case where the table look up fails?
Good point, I'll expand the table then the large vector cases won't be found in the table.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/ctpop.ll:168
 ; CHECK-LABEL: 'test_ctpop_v32i8'
-; CHECK-NEXT:  Cost Model: Found an estimated cost of 4 for instruction: %ctpop = call <32 x i8> @llvm.ctpop.v32i8(<32 x i8> %a)
+; CHECK-NEXT:  Cost Model: Found an estimated cost of 2 for instruction: %ctpop = call <32 x i8> @llvm.ctpop.v32i8(<32 x i8> %a)
 ; CHECK-NEXT:  Cost Model: Found an estimated cost of 0 for instruction: ret <32 x i8> %ctpop
----------------
SjoerdMeijer wrote:
> Does this really have a cost of only 2?
> 
> I guess it's 2 because `LT.first * Entry->Cost = 2 * 1 = 2`
> 
> So I am wondering if it is easier to expand the table with vector types, and if the lookup fails let it fall back to the generic function that will hopefully then assign a high cost for it.
I did try doing a check of the codegen for the large vector cases and they did seem to be double the cost, but I understand it is probably safer to expand the table so I'm happy to do that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103952