[PATCH][CodeGen] Fix a bug in bitcast optimization of DAGCombiner

[Andrea Di Biagio]

Hi James,

I think what Jiangning was trying to say is that this node:
  v2i32 BV = BUILD_VECTOR (i32 58712, i32 undef)

is not equivalent to:
  v4i16 SV = SCALAR_TO_VECTOR (i16 58712)

scalar_to_vector SV is equivalent to a
  BUILD_VECTOR(i16 58712, i16 undef, i16 undef, i16 undef)

However, the correct value should be instead:
  BUILD_VECTOR(i16 58712, i16 0, i16 undef, i16 undef).

So, the problem is not that an undef has been turned to zero. It is
actually the other way round :-).
An explicit zero has been turned into an undef value.

The problem could have been described a bit better, however I don't
think this patch is wrong. It actually looks good to me.
As a side note: it would have been much easier to review this patch on
phabricator with a bit of context :-).

A small nit: in the test file, please change "Bugzilla #23065" with
PR23065. Also, it would be nice to have a comment in the test file
where you briefly describe what was the wrong optimization performed
by the dag combiner.

Thanks,
Andrea



On Tue, Mar 31, 2015 at 11:43 AM, James Molloy <james at jamesmolloy.co.uk> wrote:
> Hi Jiangning,
>
> I don't understand your logic here - the upper lanes can be undef, right.
> But undef is a contract that means "anything". So if we zero-extend, we make
> the upper lanes zero, but zero is a valid implementation of undef.
>
> Cheers,
>
> James
>
> On Tue, 31 Mar 2015 at 10:53 Jiangning Liu <liujiangning1 at gmail.com> wrote:
>>
>> BTW, this patch is to fix https://llvm.org/bugs/show_bug.cgi?id=23065 .
>>
>> Thanks,
>> -Jiangning
>>
>> 2015-03-31 17:47 GMT+08:00 Jiangning Liu <liujiangning1 at gmail.com>:
>>>
>>> Hi,
>>>
>>> The small patch attached is to fix a bug in bitcast optimization of
>>> DAGCombiner.
>>>
>>> Originally, the code tries to optimize bitcast with build_vector input to
>>> be a scalar_to_vector, if only the bitcast and build_vector can meet some
>>> conditions. In particular, it thought "ThisVal.zext(SrcBitSize) == OpVal" is
>>> one of the requirements. This requirement means, if zero extension of a
>>> narrow element value is equal to the original wide element value, the
>>> optimization would be allowed. Unfortunately this is incorrect, because the
>>> semantic of scalar_to_vector is the top 1 to N-1 elements are undef rather
>>> than zero. So no matter we use zero_extension or sign_extension, this
>>> transformation would be always invalid. The patch attached disables this
>>> optimization.
>>>
>>> Thanks,
>>> -Jiangning
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>