Mikael Holmén via llvm-dev
2019-Nov-28 14:12 UTC
[llvm-dev] Instcombine and bitcast of vector. Wrong CHECKs in cast.ll, miscompile in instcombine?
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
Sanjay Patel via llvm-dev
2019-Nov-28 15:55 UTC
[llvm-dev] Instcombine and bitcast of vector. Wrong CHECKs in cast.ll, miscompile in instcombine?
Looks broken to me - we need to consider big/little-endian datalayout when bitcasting to/from vectors. 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://reviews.llvm.org/rL103354 You can find other vector bitcast transforms that (hopefully correctly...) account for the datalayout difference for vector elements. Example: https://github.com/llvm/llvm-project/blob/master/llvm/test/Transforms/InstCombine/bitcast-bigendian.ll#L10 https://github.com/llvm/llvm-project/blob/master/llvm/test/Transforms/InstCombine/bitcast.ll#L290 So we need something like this: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp#L487 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://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/20191128/862eccbb/attachment.html>
Björn Pettersson A via llvm-dev
2019-Nov-28 16:20 UTC
[llvm-dev] Instcombine and bitcast of vector. Wrong CHECKs in cast.ll, miscompile in instcombine?
Thanks Sanjay for confirming that this seem to be broken. I’ll check with Mikael to make sure we write a new PR (probably have to wait until tomorrow). PS. This kind of confirms that DAGTypeLegalizer::PromoteIntRes_BITCAST is doing the wrong thing for big-endian as well, see https://bugs.llvm.org/show_bug.cgi?id=44135 , so maybe we can set that PR to “confirmed” (and then move forward with making a proper patch based on the workaround presented in that PR). /Björn From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Sanjay Patel via llvm-dev Sent: den 28 november 2019 16:55 To: Mikael Holmén <mikael.holmen at ericsson.com>; Chris Lattner <clattner at nondot.org> Cc: llvm-dev at lists.llvm.org Subject: Re: [llvm-dev] Instcombine and bitcast of vector. Wrong CHECKs in cast.ll, miscompile in instcombine? Looks broken to me - we need to consider big/little-endian datalayout when bitcasting to/from vectors. 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://reviews.llvm.org/rL103354<https://protect2.fireeye.com/v1/url?k=e6ff41bf-ba756d27-e6ff0124-0cc47ad93eae-0e34bc4531ded90b&q=1&e=7537b523-36d8-43aa-9f2e-412e89ef8c5f&u=https%3A%2F%2Freviews.llvm.org%2FrL103354> You can find other vector bitcast transforms that (hopefully correctly...) account for the datalayout difference for vector elements. Example: https://github.com/llvm/llvm-project/blob/master/llvm/test/Transforms/InstCombine/bitcast-bigendian.ll#L10<https://protect2.fireeye.com/v1/url?k=4a660b74-16ec27ec-4a664bef-0cc47ad93eae-98051a8097706eba&q=1&e=7537b523-36d8-43aa-9f2e-412e89ef8c5f&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project%2Fblob%2Fmaster%2Fllvm%2Ftest%2FTransforms%2FInstCombine%2Fbitcast-bigendian.ll%23L10> https://github.com/llvm/llvm-project/blob/master/llvm/test/Transforms/InstCombine/bitcast.ll#L290<https://protect2.fireeye.com/v1/url?k=7e024c7d-228860e5-7e020ce6-0cc47ad93eae-c3ed7bf3f1423e80&q=1&e=7537b523-36d8-43aa-9f2e-412e89ef8c5f&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://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp#L487<https://protect2.fireeye.com/v1/url?k=eb400e4d-b7ca22d5-eb404ed6-0cc47ad93eae-33441214035703e7&q=1&e=7537b523-36d8-43aa-9f2e-412e89ef8c5f&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project%2Fblob%2Fmaster%2Fllvm%2Flib%2FTransforms%2FInstCombine%2FInstCombineCasts.cpp%23L487> On Thu, Nov 28, 2019 at 9:12 AM Mikael Holmén via llvm-dev <llvm-dev at lists.llvm.org<mailto: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<mailto:llvm-dev at lists.llvm.org> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev<https://protect2.fireeye.com/v1/url?k=616c8bcf-3de6a757-616ccb54-0cc47ad93eae-ec99014218a008a2&q=1&e=7537b523-36d8-43aa-9f2e-412e89ef8c5f&u=https%3A%2F%2Flists.llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fllvm-dev> -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191128/cfd6e305/attachment.html>
Mikael Holmén via llvm-dev
2019-Nov-29 06:25 UTC
[llvm-dev] Instcombine and bitcast of vector. Wrong CHECKs in cast.ll, miscompile in instcombine?
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://reviews.llvm.org/rL103354 >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://github.com/llvm/llvm-project/blob/master/llvm/test/Transforms/InstCombine/bitcast-bigendian.ll#L10>https://github.com/llvm/llvm-project/blob/master/llvm/test/Transforms/InstCombine/bitcast.ll#L290> > So we need something like this: >https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp#L487> >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://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reasonably Related Threads
- [LLVMdev] 64bit MRV problem: { float, float, float} -> { double, float }
- [LLVMdev] 64bit MRV problem: { float, float, float} -> { double, float }
- [LLVMdev] 64bit MRV problem: { float, float, float} -> { double, float }
- Priority between calls from different queues
- Outlook express confusion