[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
+  // If all of them are -1, OOB read will happen later.
+  if (std::none_of(Mask.begin(), Mask.end(), [VecTy](unsigned Idx) {
+        return Idx < VecTy->getNumElements() * 2;
----------------
fhahn wrote:
> fhahn wrote:
> > Peter wrote:
> > > Peter wrote:
> > > > fhahn wrote:
> > > > > fhahn wrote:
> > > > > > LLVM provides `any_of`/`all_of` helpers that take a iterator range. It would be good to use them here. 
> > > > > > 
> > > > > > >   // If mask is `undef` or `poison`, `Mask` may be a vector of -1s.
> > > > > > 
> > > > > > There's a `constexpr` for `UndefMaskElem`: https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/Instructions.h#L1996
> > > > > > 
> > > > > > It would probably be better to check for that. IIUC this is the only case where an index could be out-of-range; other cases would be already rejected by the verifier I think.
> > > > > Don't we want `any_of` here? Won't a single `-1` create an issue in the mask?
> > > > No, It seems a few UndefMaskElem won't affect how it works. See test case `https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/InterleavedAccess/AArch64/interleaved-accesses-inseltpoison.ll`.
> > > > 
> > > > Only a whole vector of UndefMaskElem will crash it.
> > > Correction, see test case here: https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/InterleavedAccess/AArch64/interleaved-accesses-inseltpoison.ll#L246
> > > 
> > > Seems they expected a few elements to be undef or poison when they invented this feature, but never thought if the whole mask is undef.
> > Ah right, that makes me think that the patch doesn't fully fix the underlying issue.
> > 
> > E.g. the following below should also crash
> > 
> > ```
> > define void @foo(<8 x i64> %a, ptr %dst) {
> > BB:
> >   %S = shufflevector <8 x i64> %a, <8 x i64> %a, <16 x i32> <i32 0, i32 undef, i32 undef, i32 undef,i32 undef, i32 undef, i32 undef, i32 undef,i32 undef, i32 undef, i32 undef, i32 undef,i32 undef, i32 undef, i32 undef, i32 undef>
> >   store <16 x i64> %S, ptr %dst, align 64
> >   ret void
> > }
> > ```
> If some lanes are undef, that's fine and there's already some support, the issue is that we compute an invalid index to find the start mask. But if all elements we need the select have undef indices, that's would still be fine.
> 
> There are more variations with only some undef/poison lanes that crash.
 You are right, the example you proposed is crashing. 
Now I am lostdon't think I fully grasp the reason for the crashing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132634