[PATCH] D85759: [SLPVectorizer] Fix regression from cost model refactoring

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

I took a little more look at this and all of this cost model is very confusing. :( First I don't understand why the lists of intrinsics listed in switch-cases in getIntrinsicInstrCost <https://github.com/llvm/llvm-project/blob/95fad44e34c3c20263961a715571d798d90921f6/llvm/include/llvm/CodeGen/BasicTTIImpl.h#L1123-L1124> and getTypeBasedIntrinsicInstrCost <https://github.com/llvm/llvm-project/blob/95fad44e34c3c20263961a715571d798d90921f6/llvm/include/llvm/CodeGen/BasicTTIImpl.h#L1272-L1273> are different. `fshr` is listed <https://github.com/llvm/llvm-project/blob/95fad44e34c3c20263961a715571d798d90921f6/llvm/include/llvm/CodeGen/BasicTTIImpl.h#L1227> only in `getIntrinsicInstrCost` but not in `getTypeBasedIntrinsicInstrCost`. It uses some more info than just types (like if some value is power of 2 or something), but even if we don't assume anything, it returns a far greater number because it computes and accumulates <https://github.com/llvm/llvm-project/blob/95fad44e34c3c20263961a715571d798d90921f6/llvm/include/llvm/CodeGen/BasicTTIImpl.h#L1241-L1263> cost for each of its argument while `getTypeBasedIntrinsicInstrCost` does not have a case handling for `fshr` so it just assumes it is cheap and returns <https://github.com/llvm/llvm-project/blob/95fad44e34c3c20263961a715571d798d90921f6/llvm/include/llvm/CodeGen/BasicTTIImpl.h#L1311> 1.

Not sure the code has evolved like this and I'm sure one of the paths here is wrong. It looks `getIntrinsicInstrCost` looks more correct, because it has at least a specialized handling routine for this, while `getTypeBasedIntrinsicInstrCost` blindly assumes it is cheap and returns 1. But @tlively says the previous version, which is using `getTypeBasedIntrinsicInstrCost` and returning 1, was correct. Can you elaborate why this has to be 1?

The routines here look like written by many people throughout a long period and not very consistent, so not sure what the correct action here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85759