[llvm-dev] Instcombine and bitcast of vector. Wrong CHECKs in cast.ll, miscompile in instcombine?

Thu Nov 28 23:52:27 PST 2019

I wrote

 https://bugs.llvm.org/show_bug.cgi?id=44178

about this.

Thanks,
Mikael

On Fri, 2019-11-29 at 06:25 +0000, Mikael Holmén via llvm-dev wrote:
> On Thu, 2019-11-28 at 10:55 -0500, Sanjay Patel wrote:
> > Looks broken to me - we need to consider big/little-endian
> > datalayout
> > when bitcasting to/from vectors.
> 
> Great, that's what I thought.
> 
> I ran into this problem in a real miscompile for our out-of-tree
> target, did a tentative fix to instcombine, but then noticed that
> test60 in cast.ll failed and got a bit confused/worried.
> 
> > We should have some documentation for this in the LangRef, but I
> > don't see anything currently.
> > 
> > The transform in question was added here:
> > 
https://protect2.fireeye.com/v1/url?k=0f6f3328-53e5e7e9-0f6f73b3-863d9bcb726f-48266812a3c68cf9&q=1&e=17c6716e-878a-4ce0-b201-b3607d52d122&u=https%3A%2F%2Freviews.llvm.org%2FrL103354
> > 
> 
> Yes, from 2010. :) It also added to my confusion that the bug had
> lived
> for so long when I saw the code was added in that commit, hence the
> email.
> 
> > You can find other vector bitcast transforms that (hopefully
> > correctly...) account for the datalayout difference for vector
> > elements.
> > 
> > Example:
> > 
> 
> 
https://protect2.fireeye.com/v1/url?k=ae1f2585-f295f144-ae1f651e-863d9bcb726f-eb7ff0d04afffaf6&q=1&e=17c6716e-878a-4ce0-b201-b3607d52d122&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project%2Fblob%2Fmaster%2Fllvm%2Ftest%2FTransforms%2FInstCombine%2Fbitcast-bigendian.ll%23L10
> > 
> 
> 
https://protect2.fireeye.com/v1/url?k=94cc4d6b-c84699aa-94cc0df0-863d9bcb726f-f0bb5e5b3c15906f&q=1&e=17c6716e-878a-4ce0-b201-b3607d52d122&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project%2Fblob%2Fmaster%2Fllvm%2Ftest%2FTransforms%2FInstCombine%2Fbitcast.ll%23L290
> > 
> > So we need something like this:
> > 
> 
> 
https://protect2.fireeye.com/v1/url?k=7a6c6688-26e6b249-7a6c2613-863d9bcb726f-6ad879f8e930c3ce&q=1&e=17c6716e-878a-4ce0-b201-b3607d52d122&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project%2Fblob%2Fmaster%2Fllvm%2Flib%2FTransforms%2FInstCombine%2FInstCombineCasts.cpp%23L487
> > 
> > 
> 
> Good, then I'll write a PR for the instcombine issue and broken test
> and probably also put up a patch for review.
> 
> I think test61 in cast.ll (checking what happens when we
> bitcast/zext/bitcast) is also broken, I think it inserts zeroes at
> the
> wrong end for big-endian targets so I'll include something about that
> too.
> 
> As Björn mentioned we've seen a similar issue (PR44135) in the
> DAGLegalizer so it seems vectors on big-endian machines aren't that
> well tested, at least some code patterns aren't.
> 
> We triggered both these problems after enabling unrolling for our
> target, so I suppose that resulted in some new code patterns. We'll
> see
> if anything more comes out of that.
> 
> Thanks,
> Mikael
> 
> > On Thu, Nov 28, 2019 at 9:12 AM Mikael Holmén via llvm-dev <
> > llvm-dev at lists.llvm.org> wrote:
> > > Hi,
> > > 
> > > In
> > >  llvm/test/Transforms/InstCombine/cast.ll
> > > there is a test like this:
> > > 
> > > target datalayout = "E-p:64:64:64-p1:32:32:32-p2:64:64:64-
> > > p3:64:64:64-
> > > a0:0:8-f32:32:32-f64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-
> > > i64:32:64-
> > > v64:64:64-v128:128:128-n8:16:32:64"
> > > 
> > > [...]
> > > 
> > > define <3 x i32> @test60(<4 x i32> %call4) {
> > > ; CHECK-LABEL: @test60(
> > > ; CHECK-NEXT:    [[P10:%.*]] = shufflevector <4 x i32>
> > > [[CALL4:%.*]],
> > > <4 x i32> undef, <3 x i32> <i32 0, i32 1, i32 2>
> > > ; CHECK-NEXT:    ret <3 x i32> [[P10]]
> > > ;
> > >   %p11 = bitcast <4 x i32> %call4 to i128
> > >   %p9 = trunc i128 %p11 to i96
> > >   %p10 = bitcast i96 %p9 to <3 x i32>
> > >   ret <3 x i32> %p10
> > > 
> > > }
> > > 
> > > If we assume the input vector is e.g. <1, 2, 3, 4> then I assume
> > > %p11
> > > would be the (hex) value 1234, %p9 would be the 234 and %p10
> > > would
> > > then
> > > be the vector <2, 3, 4>.
> > > 
> > > Am I right, or am I missing something here? Note that the
> > > datalayout
> > > says we're using big endian.
> > > 
> > > But the CHECK-NEXT checks that the result is made up of the
> > > elements at
> > > index 0, 1 and 2 from the input vector, which would be <1, 2, 3>.
> > > 
> > > So, broken testcase or am I missing something?
> > > 
> > > Thanks,
> > > Mikael
> > > 
> > > _______________________________________________
> > > LLVM Developers mailing list
> > > llvm-dev at lists.llvm.org
> > > 
https://protect2.fireeye.com/v1/url?k=40f8f8fa-1c722c3b-40f8b861-863d9bcb726f-0ac67e884a2a0ba9&q=1&e=17c6716e-878a-4ce0-b201-b3607d52d122&u=https%3A%2F%2Flists.llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fllvm-dev
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> 
https://protect2.fireeye.com/v1/url?k=2193cc3b-7d1918fa-21938ca0-863d9bcb726f-bf9bd0950bc61675&q=1&e=17c6716e-878a-4ce0-b201-b3607d52d122&u=https%3A%2F%2Flists.llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fllvm-dev