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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for working on this @john.brawn. I've left some suggestions on your patch.



================
Comment at: llvm/lib/CodeGen/StackProtector.cpp:355-359
+        // The exact size of scalable vectors isn't known, so go for the worst
+        // case of a single element.
+        bool isScalable = isa<ScalableVectorType>(AllocType);
+        uint64_t AllocSize = M->getDataLayout().getTypeAllocSize(
+          isScalable ? AllocType->getScalarType() : AllocType);
----------------
I'd suggest changing the interface HasAddressTaken to take a TypeSize.


================
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
----------------
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?


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1126
 
+  bool SVEBelowBP = MFI.getStackProtectorIndex() >= 0;
+
----------------
Could this use `hasStackProtectorIndex` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111631