llvm dev - Jul 2016 - Hitting assertion failure related to vectorization + instcombine

Sanjay: let me know if this is something that will apply to 3.9.

Thanks,
Hans

On Wed, Jul 20, 2016 at 5:59 PM, Sanjay Patel via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> Quick update - the bug existed before I refactored that chunk in
> InstSimplify with:
> https://reviews.llvm.org/rL275911
>
> In fact, as discussed in https://reviews.llvm.org/D22537 - because we have
a
> big pile of lazily copied code, the bug has an identical twin that exists
in
> InstCombine. I swear I didn't touch that code...yet. :)
>
> define <2 x i32> @select_icmp_vec(<2 x i32> %x) {
>   %cmp = icmp slt <2 x i32> %x, zeroinitializer
>   %xor = xor <2 x i32> %x, <i32 2147483648, i32 2147483648>
>   %x.xor = select <2 x i1> %cmp, <2 x i32> %x, <2 x i32>
%xor
>   ret <2 x i32> %x.xor
> }
>
> $ ./opt -instcombine selvec.ll -S
> Assertion failed: (BitWidth == RHS.BitWidth && "Comparison
requires equal
> bit widths"), function operator==, file
> /Users/spatel/myllvm/llvm/include/llvm/ADT/APInt.h, line 983.
>
> I should have a patch up for review shortly.
>
>
> On Wed, Jul 20, 2016 at 2:03 PM, Sanjay Patel <spatel at
rotateright.com>
> wrote:
>>
>> Thanks for notifying me. Yes, this was a recent change. Taking a look
now.
>>
>> On Wed, Jul 20, 2016 at 1:49 PM, Michael Kuperstein <mkuper at
google.com>
>> wrote:
>>>
>>> +Sanjay, who touched this last. :-)
>>>
>>> On Wed, Jul 20, 2016 at 12:44 PM, Ismail Badawi (ibadawi) via
llvm-dev
>>> <llvm-dev at lists.llvm.org> wrote:
>>>>
>>>> Hi folks,
>>>>
>>>> I'm hitting the below assertion failure when compiling this
small piece
>>>> of C code (repro.c, attached).
>>>>
>>>> My command line is:
>>>>
>>>> bin/clang --target=aarch64-linux-gnu -c -O2 repro.c
>>>>
>>>> clang is built from top of trunk as of this morning. It only
happens at
>>>> -O2, and it doesn't happen with the default target
(x86_64). I tried to
>>>> reproduce using just 'llc -O2' but didn't manage --
but I do have this
>>>> reduced opt command line (repro.ll also attached, just
generated from
>>>> repro.c at -O0):
>>>>
>>>> bin/opt -instcombine -licm -simplifycfg -instcombine
-loop-rotate
>>>> -loop-vectorize -instcombine < repro.ll
>>>>
>>>> The failure is:
>>>>
>>>> opt:
/scratch/1/ismail/llvm-upstream/include/llvm/ADT/APInt.h:983: bool
>>>> llvm::APInt::operator==(const llvm::APInt&) const:
Assertion `BitWidth =>>>> RHS.BitWidth && "Comparison
requires equal bit widths"' failed.
>>>> ...snip...
>>>> #8 0x00000000013a8553 llvm::APInt::operator==(llvm::APInt
const&) const
>>>> /scratch/1/ismail/llvm-upstream/include/llvm/ADT/APInt.h:984:0
>>>> #9 0x0000000001f875b0 simplifySelectBitTest(llvm::Value*,
llvm::Value*,
>>>> llvm::Value*, llvm::APInt const*, bool)
>>>>
/scratch/1/ismail/llvm-upstream/lib/Analysis/InstructionSimplify.cpp:3388:0
>>>> #10 0x0000000001f87ad5 simplifySelectWithICmpCond(llvm::Value*,
>>>> llvm::Value*, llvm::Value*, (anonymous namespace)::Query
const&, unsigned
>>>> int)
>>>>
/scratch/1/ismail/llvm-upstream/lib/Analysis/InstructionSimplify.cpp:3434:0
>>>> #11 0x0000000001f87f6a SimplifySelectInst(llvm::Value*,
llvm::Value*,
>>>> llvm::Value*, (anonymous namespace)::Query const&, unsigned
int)
>>>>
/scratch/1/ismail/llvm-upstream/lib/Analysis/InstructionSimplify.cpp:3515:0
>>>> #12 0x0000000001f87fe3 llvm::SimplifySelectInst(llvm::Value*,
>>>> llvm::Value*, llvm::Value*, llvm::DataLayout const&,
llvm::TargetLibraryInfo
>>>> const*, llvm::DominatorTree const*, llvm::AssumptionCache*,
>>>> llvm::Instruction const*)
>>>>
/scratch/1/ismail/llvm-upstream/lib/Analysis/InstructionSimplify.cpp:3528:0
>>>> ...snip...
>>>> Stack dump:
>>>> 0.  Program arguments: debugbuild/bin/opt -instcombine -licm
>>>> -simplifycfg -instcombine -loop-rotate -loop-vectorize
-instcombine
>>>> 1.  Running pass 'Function Pass Manager' on module
'<stdin>'.
>>>> 2.  Running pass 'Combine redundant instructions' on
function '@strsave'
>>>>
>>>> ---
>>>>
>>>> Looking at the code, the issue is with this line:
>>>>
>>>>   if (TrueVal == X && match(FalseVal,
m_And(m_Specific(X), m_APInt(C)))
>>>> &&
>>>>         *Y == ~*C)
>>>>
>>>> In this case Y is a 128-bit APInt, and so is the value that C
is
>>>> extracted from, but the m_APInt matcher has code that calls
getSplatValue()
>>>> if the matched expression has vector type. So C ends up as an
8-bit value,
>>>> triggering the assertion failure on the call to ==.
>>>>
>>>> The issue is clear but I'm not sure what the correct fix
should be --
>>>> whether this code is just not meant to work with vector types,
or whether
>>>> the call to getSplatValue() doesn't belong in the matcher,
or whether there
>>>> should be a corresponding call to getSplatValue() on the caller
side, or
>>>> something else.
>>>>
>>>> Any help from someone with more context would be appreciated.
>>>>
>>>> Thanks,
>>>> Ismail
>>>>
>>>>
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>>
>>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

Hi Hans -

Yes, I think this is a good patch for 3.9 (cc'ing David Majnemer as code
owner). The functional change was r276209. I made an NFC refactoring in
InstSimplify at r275911 that isn't in the branch, so I don't think the
patch will apply as-is. For safety, you could apply the one-line fix
without pulling the refactoring into the branch with this diff:

-    unsigned BitWidth = Q.DL.getTypeSizeInBits(TrueVal->getType());
+    unsigned BitWidth +       
Q.DL.getTypeSizeInBits(TrueVal->getType()->getScalarType());


<https://reviews.llvm.org/rL275911>

On Fri, Jul 22, 2016 at 7:45 AM, Hans Wennborg <hans at chromium.org>
wrote:
> Sanjay: let me know if this is something that will apply to 3.9.
>
> Thanks,
> Hans
>
> On Wed, Jul 20, 2016 at 5:59 PM, Sanjay Patel via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > Quick update - the bug existed before I refactored that chunk in
> > InstSimplify with:
> > https://reviews.llvm.org/rL275911
> >
> > In fact, as discussed in https://reviews.llvm.org/D22537 - because we
> have a
> > big pile of lazily copied code, the bug has an identical twin that
> exists in
> > InstCombine. I swear I didn't touch that code...yet. :)
> >
> > define <2 x i32> @select_icmp_vec(<2 x i32> %x) {
> >   %cmp = icmp slt <2 x i32> %x, zeroinitializer
> >   %xor = xor <2 x i32> %x, <i32 2147483648, i32
2147483648>
> >   %x.xor = select <2 x i1> %cmp, <2 x i32> %x, <2 x
i32> %xor
> >   ret <2 x i32> %x.xor
> > }
> >
> > $ ./opt -instcombine selvec.ll -S
> > Assertion failed: (BitWidth == RHS.BitWidth &&
"Comparison requires equal
> > bit widths"), function operator==, file
> > /Users/spatel/myllvm/llvm/include/llvm/ADT/APInt.h, line 983.
> >
> > I should have a patch up for review shortly.
> >
> >
> > On Wed, Jul 20, 2016 at 2:03 PM, Sanjay Patel <spatel at
rotateright.com>
> > wrote:
> >>
> >> Thanks for notifying me. Yes, this was a recent change. Taking a
look
> now.
> >>
> >> On Wed, Jul 20, 2016 at 1:49 PM, Michael Kuperstein <mkuper at
google.com>
> >> wrote:
> >>>
> >>> +Sanjay, who touched this last. :-)
> >>>
> >>> On Wed, Jul 20, 2016 at 12:44 PM, Ismail Badawi (ibadawi) via
llvm-dev
> >>> <llvm-dev at lists.llvm.org> wrote:
> >>>>
> >>>> Hi folks,
> >>>>
> >>>> I'm hitting the below assertion failure when compiling
this small
> piece
> >>>> of C code (repro.c, attached).
> >>>>
> >>>> My command line is:
> >>>>
> >>>> bin/clang --target=aarch64-linux-gnu -c -O2 repro.c
> >>>>
> >>>> clang is built from top of trunk as of this morning. It
only happens
> at
> >>>> -O2, and it doesn't happen with the default target
(x86_64). I tried
> to
> >>>> reproduce using just 'llc -O2' but didn't
manage -- but I do have this
> >>>> reduced opt command line (repro.ll also attached, just
generated from
> >>>> repro.c at -O0):
> >>>>
> >>>> bin/opt -instcombine -licm -simplifycfg -instcombine
-loop-rotate
> >>>> -loop-vectorize -instcombine < repro.ll
> >>>>
> >>>> The failure is:
> >>>>
> >>>> opt:
/scratch/1/ismail/llvm-upstream/include/llvm/ADT/APInt.h:983:
> bool
> >>>> llvm::APInt::operator==(const llvm::APInt&) const:
Assertion
> `BitWidth => >>>> RHS.BitWidth && "Comparison
requires equal bit widths"' failed.
> >>>> ...snip...
> >>>> #8 0x00000000013a8553 llvm::APInt::operator==(llvm::APInt
const&)
> const
> >>>>
/scratch/1/ismail/llvm-upstream/include/llvm/ADT/APInt.h:984:0
> >>>> #9 0x0000000001f875b0 simplifySelectBitTest(llvm::Value*,
> llvm::Value*,
> >>>> llvm::Value*, llvm::APInt const*, bool)
> >>>>
> /scratch/1/ismail/llvm-upstream/lib/Analysis/InstructionSimplify.cpp:3388:0
> >>>> #10 0x0000000001f87ad5
simplifySelectWithICmpCond(llvm::Value*,
> >>>> llvm::Value*, llvm::Value*, (anonymous namespace)::Query
const&,
> unsigned
> >>>> int)
> >>>>
> /scratch/1/ismail/llvm-upstream/lib/Analysis/InstructionSimplify.cpp:3434:0
> >>>> #11 0x0000000001f87f6a SimplifySelectInst(llvm::Value*,
llvm::Value*,
> >>>> llvm::Value*, (anonymous namespace)::Query const&,
unsigned int)
> >>>>
> /scratch/1/ismail/llvm-upstream/lib/Analysis/InstructionSimplify.cpp:3515:0
> >>>> #12 0x0000000001f87fe3
llvm::SimplifySelectInst(llvm::Value*,
> >>>> llvm::Value*, llvm::Value*, llvm::DataLayout const&,
> llvm::TargetLibraryInfo
> >>>> const*, llvm::DominatorTree const*,
llvm::AssumptionCache*,
> >>>> llvm::Instruction const*)
> >>>>
> /scratch/1/ismail/llvm-upstream/lib/Analysis/InstructionSimplify.cpp:3528:0
> >>>> ...snip...
> >>>> Stack dump:
> >>>> 0.  Program arguments: debugbuild/bin/opt -instcombine
-licm
> >>>> -simplifycfg -instcombine -loop-rotate -loop-vectorize
-instcombine
> >>>> 1.  Running pass 'Function Pass Manager' on module
'<stdin>'.
> >>>> 2.  Running pass 'Combine redundant instructions'
on function
> '@strsave'
> >>>>
> >>>> ---
> >>>>
> >>>> Looking at the code, the issue is with this line:
> >>>>
> >>>>   if (TrueVal == X && match(FalseVal,
m_And(m_Specific(X),
> m_APInt(C)))
> >>>> &&
> >>>>         *Y == ~*C)
> >>>>
> >>>> In this case Y is a 128-bit APInt, and so is the value
that C is
> >>>> extracted from, but the m_APInt matcher has code that
calls
> getSplatValue()
> >>>> if the matched expression has vector type. So C ends up as
an 8-bit
> value,
> >>>> triggering the assertion failure on the call to ==.
> >>>>
> >>>> The issue is clear but I'm not sure what the correct
fix should be --
> >>>> whether this code is just not meant to work with vector
types, or
> whether
> >>>> the call to getSplatValue() doesn't belong in the
matcher, or whether
> there
> >>>> should be a corresponding call to getSplatValue() on the
caller side,
> or
> >>>> something else.
> >>>>
> >>>> Any help from someone with more context would be
appreciated.
> >>>>
> >>>> Thanks,
> >>>> Ismail
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> LLVM Developers mailing list
> >>>> llvm-dev at lists.llvm.org
> >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >>>>
> >>>
> >>
> >
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160722/568ceeab/attachment.html>

Sure. David, what do you think about merging this to 3.9?

Sanjay: are you saying I'd just apply that diff to
InstructionSimplify.cpp, not InstCombineSelect.cpp?

On Fri, Jul 22, 2016 at 7:08 AM, Sanjay Patel <spatel at rotateright.com>
wrote:
> Hi Hans -
>
> Yes, I think this is a good patch for 3.9 (cc'ing David Majnemer as
code
> owner). The functional change was r276209. I made an NFC refactoring in
> InstSimplify at r275911 that isn't in the branch, so I don't think
the patch
> will apply as-is. For safety, you could apply the one-line fix without
> pulling the refactoring into the branch with this diff:
>
> -    unsigned BitWidth = Q.DL.getTypeSizeInBits(TrueVal->getType());
> +    unsigned BitWidth > +       
Q.DL.getTypeSizeInBits(TrueVal->getType()->getScalarType());
>
>
>
>
> On Fri, Jul 22, 2016 at 7:45 AM, Hans Wennborg <hans at chromium.org>
wrote:
>>
>> Sanjay: let me know if this is something that will apply to 3.9.
>>
>> Thanks,
>> Hans
>>
>> On Wed, Jul 20, 2016 at 5:59 PM, Sanjay Patel via llvm-dev
>> <llvm-dev at lists.llvm.org> wrote:
>> > Quick update - the bug existed before I refactored that chunk in
>> > InstSimplify with:
>> > https://reviews.llvm.org/rL275911
>> >
>> > In fact, as discussed in https://reviews.llvm.org/D22537 - because
we
>> > have a
>> > big pile of lazily copied code, the bug has an identical twin that
>> > exists in
>> > InstCombine. I swear I didn't touch that code...yet. :)
>> >
>> > define <2 x i32> @select_icmp_vec(<2 x i32> %x) {
>> >   %cmp = icmp slt <2 x i32> %x, zeroinitializer
>> >   %xor = xor <2 x i32> %x, <i32 2147483648, i32
2147483648>
>> >   %x.xor = select <2 x i1> %cmp, <2 x i32> %x, <2 x
i32> %xor
>> >   ret <2 x i32> %x.xor
>> > }
>> >
>> > $ ./opt -instcombine selvec.ll -S
>> > Assertion failed: (BitWidth == RHS.BitWidth &&
"Comparison requires
>> > equal
>> > bit widths"), function operator==, file
>> > /Users/spatel/myllvm/llvm/include/llvm/ADT/APInt.h, line 983.
>> >
>> > I should have a patch up for review shortly.
>> >
>> >
>> > On Wed, Jul 20, 2016 at 2:03 PM, Sanjay Patel <spatel at
rotateright.com>
>> > wrote:
>> >>
>> >> Thanks for notifying me. Yes, this was a recent change. Taking
a look
>> >> now.
>> >>
>> >> On Wed, Jul 20, 2016 at 1:49 PM, Michael Kuperstein <mkuper
at google.com>
>> >> wrote:
>> >>>
>> >>> +Sanjay, who touched this last. :-)
>> >>>
>> >>> On Wed, Jul 20, 2016 at 12:44 PM, Ismail Badawi (ibadawi)
via llvm-dev
>> >>> <llvm-dev at lists.llvm.org> wrote:
>> >>>>
>> >>>> Hi folks,
>> >>>>
>> >>>> I'm hitting the below assertion failure when
compiling this small
>> >>>> piece
>> >>>> of C code (repro.c, attached).
>> >>>>
>> >>>> My command line is:
>> >>>>
>> >>>> bin/clang --target=aarch64-linux-gnu -c -O2 repro.c
>> >>>>
>> >>>> clang is built from top of trunk as of this morning.
It only happens
>> >>>> at
>> >>>> -O2, and it doesn't happen with the default target
(x86_64). I tried
>> >>>> to
>> >>>> reproduce using just 'llc -O2' but didn't
manage -- but I do have
>> >>>> this
>> >>>> reduced opt command line (repro.ll also attached, just
generated from
>> >>>> repro.c at -O0):
>> >>>>
>> >>>> bin/opt -instcombine -licm -simplifycfg -instcombine
-loop-rotate
>> >>>> -loop-vectorize -instcombine < repro.ll
>> >>>>
>> >>>> The failure is:
>> >>>>
>> >>>> opt:
/scratch/1/ismail/llvm-upstream/include/llvm/ADT/APInt.h:983:
>> >>>> bool
>> >>>> llvm::APInt::operator==(const llvm::APInt&) const:
Assertion
>> >>>> `BitWidth =>> >>>> RHS.BitWidth
&& "Comparison requires equal bit widths"' failed.
>> >>>> ...snip...
>> >>>> #8 0x00000000013a8553
llvm::APInt::operator==(llvm::APInt const&)
>> >>>> const
>> >>>>
/scratch/1/ismail/llvm-upstream/include/llvm/ADT/APInt.h:984:0
>> >>>> #9 0x0000000001f875b0
simplifySelectBitTest(llvm::Value*,
>> >>>> llvm::Value*,
>> >>>> llvm::Value*, llvm::APInt const*, bool)
>> >>>>
>> >>>>
/scratch/1/ismail/llvm-upstream/lib/Analysis/InstructionSimplify.cpp:3388:0
>> >>>> #10 0x0000000001f87ad5
simplifySelectWithICmpCond(llvm::Value*,
>> >>>> llvm::Value*, llvm::Value*, (anonymous
namespace)::Query const&,
>> >>>> unsigned
>> >>>> int)
>> >>>>
>> >>>>
/scratch/1/ismail/llvm-upstream/lib/Analysis/InstructionSimplify.cpp:3434:0
>> >>>> #11 0x0000000001f87f6a
SimplifySelectInst(llvm::Value*, llvm::Value*,
>> >>>> llvm::Value*, (anonymous namespace)::Query const&,
unsigned int)
>> >>>>
>> >>>>
/scratch/1/ismail/llvm-upstream/lib/Analysis/InstructionSimplify.cpp:3515:0
>> >>>> #12 0x0000000001f87fe3
llvm::SimplifySelectInst(llvm::Value*,
>> >>>> llvm::Value*, llvm::Value*, llvm::DataLayout
const&,
>> >>>> llvm::TargetLibraryInfo
>> >>>> const*, llvm::DominatorTree const*,
llvm::AssumptionCache*,
>> >>>> llvm::Instruction const*)
>> >>>>
>> >>>>
/scratch/1/ismail/llvm-upstream/lib/Analysis/InstructionSimplify.cpp:3528:0
>> >>>> ...snip...
>> >>>> Stack dump:
>> >>>> 0.  Program arguments: debugbuild/bin/opt -instcombine
-licm
>> >>>> -simplifycfg -instcombine -loop-rotate -loop-vectorize
-instcombine
>> >>>> 1.  Running pass 'Function Pass Manager' on
module '<stdin>'.
>> >>>> 2.  Running pass 'Combine redundant
instructions' on function
>> >>>> '@strsave'
>> >>>>
>> >>>> ---
>> >>>>
>> >>>> Looking at the code, the issue is with this line:
>> >>>>
>> >>>>   if (TrueVal == X && match(FalseVal,
m_And(m_Specific(X),
>> >>>> m_APInt(C)))
>> >>>> &&
>> >>>>         *Y == ~*C)
>> >>>>
>> >>>> In this case Y is a 128-bit APInt, and so is the value
that C is
>> >>>> extracted from, but the m_APInt matcher has code that
calls
>> >>>> getSplatValue()
>> >>>> if the matched expression has vector type. So C ends
up as an 8-bit
>> >>>> value,
>> >>>> triggering the assertion failure on the call to ==.
>> >>>>
>> >>>> The issue is clear but I'm not sure what the
correct fix should be --
>> >>>> whether this code is just not meant to work with
vector types, or
>> >>>> whether
>> >>>> the call to getSplatValue() doesn't belong in the
matcher, or whether
>> >>>> there
>> >>>> should be a corresponding call to getSplatValue() on
the caller side,
>> >>>> or
>> >>>> something else.
>> >>>>
>> >>>> Any help from someone with more context would be
appreciated.
>> >>>>
>> >>>> Thanks,
>> >>>> Ismail
>> >>>>
>> >>>>
>> >>>> _______________________________________________
>> >>>> LLVM Developers mailing list
>> >>>> llvm-dev at lists.llvm.org
>> >>>>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> >>>>
>> >>>
>> >>
>> >
>> >
>> > _______________________________________________
>> > LLVM Developers mailing list
>> > llvm-dev at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> >
>
>