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

[Jiangning Liu]

Hi James,

Yes, Andrea's explanation is clearer than mine. Thanks a lot!

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.

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.

The patch was committed as r233778.

Thanks,
-Jiangning

2015-03-31 19:15 GMT+08:00 Andrea Di Biagio <andrea.dibiagio at gmail.com>:

> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150401/9172ff0b/attachment.html>