[PATCH] D92094: [CostModel]Replace FixedVectorType by VectorType in costgetIntrinsicInstrCost

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.

In D92094#2431032 <https://reviews.llvm.org/D92094#2431032>, @CarolineConcatto wrote:

> Hi @ctetreau.
> I don't know if you are asking tests for Scalable Vector or FixedVectorType.

As far as I can tell, this patch is only a behavioral change for scalable vectors, so those are the only tests the I'm asking about.

> For Scalable Vector:
> I added on the commit message which intrinsics will follow this patch because these intrinsics are broken for scalable vector. 
> So new tests with scalable vector will fail.
> The only intrinsic that works now for scalable vector is CCTT and CTLZ.

Prior to your change, this code bails before trying to evaluate the cost of any of the intrinsics in the switch. Now, the function will try to evaluate all of the intrinsics in the switch, but you're saying that of those, only CCTT and CTLZ actually work, and are being tested. I see this as a regression.

Please move the `if (isa<ScalableVectorType>(RetTy)) return BaseT::getIntrinsicInstrCost(ICA, CostKind);` bailing out logic into the beginning of each switch branch that does not work/is not currently tested. These can be removed on a case-by-case basis in the patches that fix these branches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92094