[PATCH] D126885: [SLP]Cost for a constant buildvector.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Coming into this a bit late.

I stumbled into this myself when looking at the impact of SLP on RISC-V.  I think this is addressing an important problem, but I'm really not happy with the structure of the change that landed.

We have a general problem here of needing to account for cost of a constant build vector.  This change ended up being specific to stores of constant build vectors, but the same basic problem still exists if e.g. you have a load, add constant-build-vector, and store sequence which gets vectorized.  The problem here is not in any way related to the cost of the store; it's related to the cost of materializing the value to be stored.

There's an additional problem that the cost model added for RISC-V is way overly simplistic.  It's out of sync with the existing build vector lowering code, and thus will result in costs which differ from the actual lowering chosen.  More importantly, the interface chosen in this patch prevents a more sophisticated cost model from being used.

I think we need to undo this, and return to the getConstBuildVectorInstrCost approach used in early versions of this patch.  There was a mention of existing build vector costing in getConstBuildVectorInstrCost, but I can't find this in generic code.  Can you point me to the code you were referring to?

p.s. I used the word "undo" specifically to avoid "revert".  I'm not asking the change be reverted, simply that we work in the direction of a better interface overall.  Doing that will have the effect of semantically reverting the landed change, but I'm not picky about the order of operations here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126885