[PATCH] D132634: [AArch64] Add index check before lowerInterleavedStore() uses ShuffleVectorInst's mask

[Peter Rong via Phabricator via llvm-commits]

Peter added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13369
+  // later.
+  for (unsigned Idx : Mask) {
+    if (Idx >= VecTy->getNumElements() * 2) {
----------------
fhahn wrote:
> This is probably too restrictive and not really matches the comment. 
> 
> There are a few test failures with the patch, including ` LLVM.Transforms/InterleavedAccess/AArch64::interleaved-accesses.ll`
I didn't know if  `undef` or `poison`  are part of the Mask is legal. That' s why I put a restrict check here. 

If they are legal, should we just check the index every time before `Mask` is indexed? I am afraid that code won't be pretty, but that' s the only thing I can think of.


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-shuffle-undef.ll:24
+  %B = urem <32 x i16> <i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1>, <i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1, i16 -1>
+  %S = shufflevector <32 x i16> %B, <32 x i16> %B, <32 x i32> undef
+  store <32 x i16> %S, <32 x i16>* %A2, align 64
----------------
fhahn wrote:
> Peter wrote:
> > Peter wrote:
> > > fhahn wrote:
> > > > can the vectors be shortened here? Does it also crash with a `poison` mask?
> > > From my experience, it seems only lengths of 32 will trigger the bug.
> > > 
> > > I just tested again. `undef` crashes both Debug and Release build. `poison` only crashes Debug build. This just adds the mystery for me...
> > > 
> > > I checked the failed testcases in Transform, it seems they only considered when part of the Mask is undef or poison, not the whole Mask.
> > Ok I managed to find a testcase with only vector length of 16.
> > 
> > ```
> > ; ModuleID = 'PoC.ll'
> > source_filename = "M"
> > 
> > define void @f() {
> > BB:
> >   %A2 = alloca <16 x i32>, align 64
> >   %B = urem <16 x i32> <i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1>, <i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1, i32 -1>
> >   %S = shufflevector <16 x i32> %B, <16 x i32> %B, <16 x i32> undef
> >   store <16 x i32> %S, <16 x i32>* %A2, align 64
> >   ret void
> > }
> > ```
> > 
> > What I don't understand is, if we change this case's integer type from i32 to i16, `llc` will complaint, even though the instruction seems legit to me
> > 
> > ```
> > ./build-debug/bin/llc: error: ./build-debug/bin/llc: PoC.ll:8:22: error: invalid shufflevector operands
> >   %S = shufflevector <16 x i16> %B, <16 x i16> %B, <16 x i16> undef
> > ```
> > 
> > But when switching to `<16 x i32>`, it crashes...
> Could be a bit simplified further: https://llvm.godbolt.org/z/663P9z73G
> 
> Yeah, the vector of indices needs to have `i32` as element type I think.
That example is awesome. I though it needs to be a constant for it to work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132634