[PATCH] D119252: [AArch64][SVE] Fix selection failure during lowering of shuffle_vector

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/test/CodeGen/AArch64/sve-shuffle-crash.ll:1
+; RUN: llc < %s
+
----------------
paulwalker-arm wrote:
> david-arm wrote:
> > bsmith wrote:
> > > david-arm wrote:
> > > > Hi @bsmith, there don't seem to be any CHECK lines here. Could you add some please using the llvm/utils/update_llc_test_checks.py script?
> > > 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?
> > 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.
> @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.
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?


================
Comment at: llvm/test/CodeGen/AArch64/sve-shuffle-crash.ll:3
+
+target triple = "aarch64-unknown-linux-gnu"
+
----------------
paulwalker-arm wrote:
> 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?
+1


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119252/new/

https://reviews.llvm.org/D119252