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

[Michael Kuperstein via llvm-commits]

I'm really sorry for how long this back-and-forth is taking, but I think
the problem may be that you're trying to do two unrelated things in one
patch.

Could you please:

1) Post a patch that fixes the cost estimate for the scalar operation, with
a test that shows the cost is correct now. (Unless we don't have a way to
test scalar costs properly. But I think we should?). The code there is
wrong, it should definitely be fixed, and your fix is obviously correct.

2) Post a separate patch that improves the vector estimate, with an
explanation of why it's an improvement, and a test that shows what exactly
improved.

(Unless there's a reason I'm missing to do 1 + 2 in the same patch. Will we
get performance regression if we only commit 1?)

Michael

On Fri, Nov 18, 2016 at 9:39 PM, Alexey Bataev <a.bataev at hotmail.com> wrote:

> No, it is not quite so. The original rationale was to fix cost estimation
> for the scalar operations (VectorType was used rather than ScalarType for
> the cost estimation of the scalar operations). But also I tried to tweak
> the model somehow to get more correct cost estimation of the vector and
> scalar operations. But I did not take into account the cost of BoUpSLP
> tree, which is part of this estimation. I will look at the cost of the tree
> and also will try to improve it, because now I think this cost is the
> reason of too optimistic cost of vectorized code.
>
> Best regards,
> Alexey Bataev
>
> > 19 нояб. 2016 г., в 2:35, Michael Kuperstein via llvm-commits <
> llvm-commits at lists.llvm.org> написал(а):
> >
> > mkuper added a comment.
> >
> > In https://reviews.llvm.org/D26277#599995, @ABataev wrote:
> >
> >> In https://reviews.llvm.org/D26277#598814, @mkuper wrote:
> >>
> >>> So, now, none of the costs on the tests actually change?
> >>> Does that mean that the changes in costs in the previous versions came
> from the +1?
> >>>
> >>> Can you add a test that demonstrates the cost change? (Preferably in a
> way that shows what happened - e.g. commit a test with the "bad" cost, and
> then have a diff with the good one).
> >>
> >>
> >> Changed a code a little bit. Checked it, the calculated cost is very
> close to the real situation, but seems to me the BoUpSLP tree cost is too
> optimistic. Will look at it later.
> >
> >
> > I'm sorry, but I'm still confused.
> > The original rationale for this patch was that the vector cost model is
> too optimistic, but the only test change seems to show the cost model
> becoming  even more optimistic.
> >
> >
> > https://reviews.llvm.org/D26277
> >
> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161121/e93fa8ae/attachment.html>