<div dir="ltr">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. <div><br></div><div>Could you please:<div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>(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?)<br></div></div><div><br></div><div>Michael</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 18, 2016 at 9:39 PM, Alexey Bataev <span dir="ltr"><<a href="mailto:a.bataev@hotmail.com" target="_blank">a.bataev@hotmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br>
<br>
Best regards,<br>
Alexey Bataev<br>
<br>
> 19 нояб. 2016 г., в 2:35, Michael Kuperstein via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> написал(а):<br>
<div class="HOEnZb"><div class="h5">><br>
> mkuper added a comment.<br>
><br>
> In <a href="https://reviews.llvm.org/D26277#599995" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D26277#599995</a>, @ABataev wrote:<br>
><br>
>> In <a href="https://reviews.llvm.org/D26277#598814" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D26277#598814</a>, @mkuper wrote:<br>
>><br>
>>> So, now, none of the costs on the tests actually change?<br>
>>> Does that mean that the changes in costs in the previous versions came from the +1?<br>
>>><br>
>>> 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).<br>
>><br>
>><br>
>> 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.<br>
><br>
><br>
> I'm sorry, but I'm still confused.<br>
> 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.<br>
><br>
><br>
> <a href="https://reviews.llvm.org/D26277" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D26277</a><br>
><br>
><br>
><br>
><br>
</div></div></blockquote></div><br></div>