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

Fri Aug 19 08:53:10 PDT 2022

ABataev added a comment.

In D126885#3735457 <https://reviews.llvm.org/D126885#3735457>, @reames wrote:

> 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?

Check the cost model of arithmetic instructions etc, they already include the cost analysis for constant values.

> 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.

I tried initially to implement it but our cost model already includes the cost of constants/constant buildvectors for many operations. It requires significant rework of TTI and some extra investigation because we need to account cross dependency between constants and the operations. And I'm not sure if it would better/easier to implement, it requires some extra (re)design investigation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126885