[PATCH] D26277: [SLP] Fixed cost model for horizontal reduction.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

In https://reviews.llvm.org/D26277#604897, @mkuper wrote:

> The point I'm trying to make is that there's still no adequate test coverage for this patch.
>
> Could you please split this into two patches:
>
> 1. A patch that contains line 4286, and a test where the behavior actually changes due to that line.
> 2. A patch that contains getReductionCost() and a test where the behavior changes due to those changes. I assume (a) the change in reduction.ll you have in this patch is due to this part, and (b) 11 is a better cost here than 17, but it seems like <8 x i32> should be legal on AVX2, and illegal on SSE/AVX, so I wonder why we get the same cost for both case. Would a smaller fix achieve the same result? If so, can you add a test that demonstrates where the logic you've added for illegal types matters.




1. Michael, I don't think this is a good idea. Actually, all these changes are required for cost model fix. I removed all optimizations from the patch, now it is a pure bug fix.
2. Yes, the patch may reduce the cost of vector operations, but also it reduces the cost of scalar operations because of use of ScalarTy instead of VectorTy.

Currently, this kind of reduction is also allowed. The vector cost of this operation is 17, but the scalar cost is 16 (because of using of VectorTy). With this patch, the vector cost is 11, but the scalar cost will be 7, so the difference even better than in the original code.


https://reviews.llvm.org/D26277