<div dir="ltr">Hi James,<div><br></div><div>Yes, Andrea's explanation is clearer than mine. Thanks a lot!</div><div><br></div><div>For that case, the original semantic give all zero for lane 1 of v4i16 eventually, but after optimization of using scalar_to_vector, it is changed to be undef, so some semantic is lost, and scalar_to_vector can't cover the original semantic.</div><div><br></div><div>If we change zext to be sext, the test I attached should still pass, but it would be still incorrect if the original constant is less than 32768. This is why I said neither zext nor sext can work, so I added one more test to check this scenario as well.</div><div><br></div><div>The patch was committed as r233778.</div><div><br></div><div>Thanks,</div><div>-Jiangning</div></div><div class="gmail_extra"><br><div class="gmail_quote">2015-03-31 19:15 GMT+08:00 Andrea Di Biagio <span dir="ltr"><<a href="mailto:andrea.dibiagio@gmail.com" target="_blank">andrea.dibiagio@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi James,<br>
<br>
I think what Jiangning was trying to say is that this node:<br>
  v2i32 BV = BUILD_VECTOR (i32 58712, i32 undef)<br>
<br>
is not equivalent to:<br>
  v4i16 SV = SCALAR_TO_VECTOR (i16 58712)<br>
<br>
scalar_to_vector SV is equivalent to a<br>
  BUILD_VECTOR(i16 58712, i16 undef, i16 undef, i16 undef)<br>
<br>
However, the correct value should be instead:<br>
  BUILD_VECTOR(i16 58712, i16 0, i16 undef, i16 undef).<br>
<br>
So, the problem is not that an undef has been turned to zero. It is<br>
actually the other way round :-).<br>
An explicit zero has been turned into an undef value.<br>
<br>
The problem could have been described a bit better, however I don't<br>
think this patch is wrong. It actually looks good to me.<br>
As a side note: it would have been much easier to review this patch on<br>
phabricator with a bit of context :-).<br>
<br>
A small nit: in the test file, please change "Bugzilla #23065" with<br>
PR23065. Also, it would be nice to have a comment in the test file<br>
where you briefly describe what was the wrong optimization performed<br>
by the dag combiner.<br>
<br>
Thanks,<br>
Andrea<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
On Tue, Mar 31, 2015 at 11:43 AM, James Molloy <<a href="mailto:james@jamesmolloy.co.uk">james@jamesmolloy.co.uk</a>> wrote:<br>
> Hi Jiangning,<br>
><br>
> I don't understand your logic here - the upper lanes can be undef, right.<br>
> But undef is a contract that means "anything". So if we zero-extend, we make<br>
> the upper lanes zero, but zero is a valid implementation of undef.<br>
><br>
> Cheers,<br>
><br>
> James<br>
><br>
> On Tue, 31 Mar 2015 at 10:53 Jiangning Liu <<a href="mailto:liujiangning1@gmail.com">liujiangning1@gmail.com</a>> wrote:<br>
>><br>
>> BTW, this patch is to fix <a href="https://llvm.org/bugs/show_bug.cgi?id=23065" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=23065</a> .<br>
>><br>
>> Thanks,<br>
>> -Jiangning<br>
>><br>
>> 2015-03-31 17:47 GMT+08:00 Jiangning Liu <<a href="mailto:liujiangning1@gmail.com">liujiangning1@gmail.com</a>>:<br>
>>><br>
>>> Hi,<br>
>>><br>
>>> The small patch attached is to fix a bug in bitcast optimization of<br>
>>> DAGCombiner.<br>
>>><br>
>>> Originally, the code tries to optimize bitcast with build_vector input to<br>
>>> be a scalar_to_vector, if only the bitcast and build_vector can meet some<br>
>>> conditions. In particular, it thought "ThisVal.zext(SrcBitSize) == OpVal" is<br>
>>> one of the requirements. This requirement means, if zero extension of a<br>
>>> narrow element value is equal to the original wide element value, the<br>
>>> optimization would be allowed. Unfortunately this is incorrect, because the<br>
>>> semantic of scalar_to_vector is the top 1 to N-1 elements are undef rather<br>
>>> than zero. So no matter we use zero_extension or sign_extension, this<br>
>>> transformation would be always invalid. The patch attached disables this<br>
>>> optimization.<br>
>>><br>
>>> Thanks,<br>
>>> -Jiangning<br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
</div></div></blockquote></div><br></div>