The lowering code for shuffle_vector has a code path that looks through
extract_subvector, this code path did not properly account for the
potential presense of larger than Neon vector types and could produce
unselectable DAG nodes.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
llvm/test/CodeGen/AArch64/sve-shuffle-crash.ll | ||
---|---|---|
1 ↗ | (On Diff #406836) | I intentionally didn't put any in here, the resulting codegen is large and uninteresting in this case, the failure mode is a crash which will be caught regardless. I can put some in though if you think it would be valuable? |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
9732 | You're introducing a new TypeSize -> uint64_t implicit typecast here. Can you use this instead? if (Extract.getOperand(0).getValueSizeInBits().getFixedSizeInBits() > 128) | |
llvm/test/CodeGen/AArch64/sve-shuffle-crash.ll | ||
1 ↗ | (On Diff #406836) | Hmm, I do realise the CHECK lines probably look awful, but maybe it's still worth it? I imagine if the code quality is really that terrible, we'll probably want to improve that at some point and at least we'll be defending it. If we improve codegen in future we'll see reduced CHECK lines here. |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
9732 | Looking at the following code can this be tightened to if (!V.getOperand(0).getValueType().is128BitVector())? It's only used post type legalisation so I think there's no other type that would be acceptable by this point. | |
llvm/test/CodeGen/AArch64/sve-shuffle-crash.ll | ||
3 ↗ | (On Diff #406836) | Not really against this file but there already exists sve-fixed-length-shuffles.ll for a similar "ensure the compiler doesn't crash on this shuffle" test and so perhaps this test can be added to that file? |
llvm/test/CodeGen/AArch64/sve-shuffle-crash.ll | ||
---|---|---|
1 ↗ | (On Diff #406836) | @david-arm Given the test relies on <32 x i32> when the target vector length is 256-bit, I'm not sure we'll ever care about the code quality. If work is done to specifically improve it I would expect there to be a more focused test. Not saying your request is wrong, just something to consider. |
llvm/test/CodeGen/AArch64/sve-shuffle-crash.ll | ||
---|---|---|
1 ↗ | (On Diff #406836) | OK fair enough. I guess I'm more worried about us missing something by not having any CHECK lines at all. Currently this test is just making sure we don't crash. I guess if @bsmith moves the test to another file it will at least need a CHECK-LABEL. :) Perhaps it would be good to have one CHECK line for a load and one for a store just to show we've generated something? |
3 ↗ | (On Diff #406836) | +1 |
Hi @paulwalker-arm, this bug was actually found in user-written code in Gromacs, although only there was only one instance of this I think. So it is something users may see, just not very often!
- Tighten type size check
- Move test to already existing file
- Add rudimentary check lines to test
llvm/test/CodeGen/AArch64/sve-fixed-length-shuffles.ll | ||
---|---|---|
18 | nit: For tests that rely on vscale_range I personally find the tests more readable if instead of referencing some attribute, it just has vscale_range(2, 2) here directly, and the RUN line has the +sve attribute. For this test it doesn't really matter too much I guess, but if the test file is long, the #<some number> make it really obscure what vscale_range it refers to. |
I found this separately (D119285) in a SPEC code, on a mixed width loop of 8b/64b vectors. But I agree, this isn't a common case.
FWIW, I can confirm the presence of ICE using the test case from https://reviews.llvm.org/D119285, https://llvm.godbolt.org/z/f1r6he545, as well as that this patch fixes the issue; thank you!
llvm/test/CodeGen/AArch64/sve-fixed-length-shuffles.ll | ||
---|---|---|
18 | I tend to agree with the first point when it comes to a single file when several different vscale_range values are being tested, although in this case all the tests are using the same value. The second point I personally disagree with because I prefer the convenience of generally being able to do llc < test.ll without having to always dig into the test file in order to run it. |
You're introducing a new TypeSize -> uint64_t implicit typecast here. Can you use this instead?