[PATCH] D102842: [Verifier] Fail on invalid indices for {insert,extract} vector intrinsics

Fri May 21 09:06:55 PDT 2021

paulwalker-arm added inline comments.

================
Comment at: llvm/test/CodeGen/AArch64/sve-insert-vector.ll:112
 ; CHECK-NEXT:    st1h { z0.h }, p0, [sp]
-; CHECK-NEXT:    stur q1, [sp, #2]
+; CHECK-NEXT:    str q1, [x9, x8]
 ; CHECK-NEXT:    ld1h { z0.h }, p0/z, [sp]
----------------
joechrisellis wrote:
> david-arm wrote:
> > This is just FYI and not caused by your patch, but suppose vscale = 1 and we clamp the index of 8 to 7. The `str q1, [x9, x8]` instruction actually ends up storing out 128 bits starting from index 7, which is beyond the end of temporary stack space. It looks like we could be corrupting the stack!
> Hi Dave, thanks for pointing this out -- this is a great spot. Had a chat with @paulwalker-arm about this and will be looking into what the possibilities are here. A key takeaway from our conversation was that if vscale = 1 for this particular insertion then we're not strictly performing an 'insert subvector', so in some sense we could consider this abuse of the intrinsic. Most of our use-cases for the intrinsic are with an idx of 0 as well, so I don't think this will cause problems if the compilation pipeline is run end-to-end on a C program or something.
> 
> Of course, we can't really defend against something like this statically, so the possibilities are limited. We discussed:
> 
> - more stringent checks at runtime -- but this could easily result in poor code quality. Perhaps there is a different/better way of checking the bounds/clamping.
> - allocating more stack space than strictly necessary to give us some breathing room (? -- might have misunderstood this).
> 
> (@paulwalker-arm, if I have misunderstood anything let me know 😄)
I'll say it's not about "breathing room" but more guaranteeing there's enough stack space, so something like `max(sizeof(ResultVT), sizeof(SubVT) + clamped(idx)*sizeof(Elt))` noting that this is only a problem when insert a fixed-length vector into a scalable vector.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102842