[PATCH] D111631: [AArch64][SVE] Fix handling of stack protection with SVE

Tue Oct 26 06:49:16 PDT 2021

john.brawn added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:110
+// way we can use VL-scaled addressing below bp to access them.
+//
 // In some cases when a base pointer is not strictly needed, it is generated
----------------
sdesmalen wrote:
> In your patch you've changed the frame-layout by swapping the areas of fixed-width locals and SVE locals and always requiring/reserving x19 as a base pointer. An alternative idea is to allocate vscale x 16 bytes (i.e. size of one SVE vector) for the stack protector slot, and have that as the first SVE slot after the SVE callee saves:
> 
>   +-----------------------+
>   |     Callee Saves      |
>   + - - - - - - - - - - - +
>   |  Frame record (LR/FP) |
>   + - - - - - - - - - - - + <- FP
>   | Neon/SVE callee saves |
>   + - - - - - - - - - - - +
>   |    Stack protector    |
>   | . . . . . . . . . . . | <- FP - sizeof(Neon/SVE callee saves) - 1*sizeof(SVE vector)
>   |                       |
>   | SVE locals/spill-slots|
>   |                       |
>   + - - - - - - - - - - - + 
>   |                       | 
>   |        Locals         | 
>   |                       | 
>   +------------------------ <- SP/BP
> 
> This way, we'd trade a bit of redundant stack space, but we'd avoid having to always reserve x19. We also have the benefit of using the same layout (reducing possibility of bugs being introduced by any new layout which is only enabled through a compile flag). Like with your current patch, we can still access SVE locals directly at `<pointer> + <ScalableOffset>`, where <pointer> in this case will be FP instead of BP.
> 
> I'm not too familiar with the exact requirements for the position of the stack protector slot, but my preliminary understanding is that it must live somewhere below the frame-record and above the locals. The above suggestion would satisfy that requirement.
> 
> Can you think of any reason why this might not be feasible?
I hadn't though about doing it this way, but it looks like it should be feasible and would definitely simplify things. I'll give it a go and see if I can get it working.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111631