[PATCH] D129994: [RISCV] Add cost modelling for vector widenning integer reduction.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D129994#3684013 <https://reviews.llvm.org/D129994#3684013>, @jacquesguan wrote:

> In D129994#3683293 <https://reviews.llvm.org/D129994#3683293>, @reames wrote:
>
>> Tests?
>>
>> Also, how are reduction opcodes other than mul and add handled?
>
> Sorry, I don't get your point, `getExtendedAddReductionCost` is only used for gettring the cost of `vecreduce.add(ext)` or `vecreduce.add(mul(ext, ext))` if `IsMLA` is true. So here we only need to handle `ADD`.

I had missed the Add in the function name and was assuming this was generic to widening reductions.  I was thinking this API would be used for the floating point widening reduction variants as well, which appears not to be the case.

Honestly, this seems like a pretty poor API design.  Not having thought about this extensively, it seems like splitting this into two APIs one which handles the generic mixed width reduction case (e.g. vecreduce.opcode(ext(Ty A))),  and one which adds the dot-product acceleration case (e.g. vecreduce.add(mul(ext(Ty A), ext(Ty B))) would make more sense.  Having a getExtendedReductionCost API would allow you to handle the FADD case, and potentially any future widen reduction instructions.  (I don't have any particular extension in mind here, just a general concern.)

Huh, actually it's worse than that.  The loop vectorizer appears to also be using this function for both reduce(ext(mul(ext(A), ext(B))) and reduce(mul(ext(A), ext(B)) - note difference in outer extend - and is inconsistent about values for MLA are passed in.  The current usage appears to contract the documented interface for the existing routine.

You should fix that before we continue with this patch.

> And for the test, I do not know well about CostModel test, `vecreduce.add(ext)` needs at least 2 instruction to make, but the test case would show the cost of each instruction, so any advice for making the case?

If nothing else, you could write a vectorizer test which checked the cost-model output.  Not ideal, but possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129994