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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

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

> In D126885#3735500 <https://reviews.llvm.org/D126885#3735500>, @ABataev wrote:
>
>> 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.
>
> I think I found the code you're referring to in X86TTIImpl::getArithmeticInstrCost.  I'd summarize this code as we have various alternate cost tables which seem to assume one constant splat operand or sometimes just one constant operand gets folded into the instruction.  I don't know enough about avx512 instruction encoding to reason about this, but I will accept that it exists.  Though it does look very weird to me that *all* constants are assumed to be folded into the encoding?  Whatever, out of scope for this discussion.
>
>>> 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.
>
> Ok, so I see the concern here.  I'm not thrilled with the conclusion, but I think I agree that the current state of the art is having each operation reason about the cost of the constant materialization independently.
>
> Given that, I see why you took the approach you did here.
>
> However, we're still left with the problem that the current interface is insufficient for RISC-V.  On the vector side, we can generate various non-splat sequences (e.g. vid and friends) at low cost.  As such, the current expressibility of interface isn't really sufficient.
>
> I see two possible paths, both with downsides.  I'm curious what you think:
>
> - Extend the OperandValueProperties enum with a bunch more options for describing build vectors.  I don't really see the semantic distinction between OperandValueProperties and OperandValueKind, so we'd probably end up merging them into a single info struct with a bunch more properties on it.  This arguably works more naturally with scalable vectors, but it's a bunch of complexity.
> - Add the getConstBuildVectorInstrCost interface anyways.  Document the contract as being to return zero cost when the constant could fold into the using instruction.  Existing backends which don't need the additional expressibility continue with the old scheme, RISC-V uses this approach to cost build vectors instead (i.e. arithmetic cost et al don't include constant mat costs).
>
> As I said, both approaches have some obvious downsides.  If you have an alternate idea, definitely open to hearing it.

I would do both (in some way) as a first step. Introduce getConstBuildVectorInstrCost (local to RiscV TTI interfac) and use it in TTI functions (I mean in getArithmeticInstrCost, getMemoryOpCost, etc.) for better constant build vector cost estimation (if the user provides operands or OperandValueProperties). Later we can make it public for all TTI interfaces. Thoughts?

> Also, to be clear, I've accepted that this patch is reasonable.  I'm asking now about future direction for my own work, not asking for you to volunteer for any of the above.  :)

I understand, no problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126885