[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 the update, the patch looks structurally good to me. Just left a few nits and a request for an extra test.



================
Comment at: llvm/lib/CodeGen/StackProtector.cpp:175
     if (MemLoc.hasValue() && MemLoc->Size.hasValue() &&
-        MemLoc->Size.getValue() > AllocSize)
+        !TypeSize::isKnownGE(AllocSize,
+                             TypeSize::getFixed(MemLoc->Size.getValue())))
----------------
nit: `TypeSize::isKnownLT` ?


================
Comment at: llvm/lib/CodeGen/StackProtector.cpp:220-221
+      TypeSize MinAllocSize = TypeSize::Fixed(AllocSize.getKnownMinValue());
+      TypeSize NewAllocSize =
+          (AllocSize.isScalable() ? MinAllocSize : AllocSize) - OffsetSize;
+      if (HasAddressTaken(I, NewAllocSize))
----------------
nit: This can just be:

  TypeSize NewAllocSize = TypeSize::Fixed(AllocSize.getKnownMinValue()) - OffsetSize;


Can you also add a test for this case? I guess that would require something like:

  %ptr = alloca <vscale x 4 x i32>
  %ptr.bc = bitcast <vscale x 4 x i32>* to i32*
  %gep = getelementptr i32, i32* %ptr.bc, i64 2 ; no stack protector needed
                                                ; (2*sizeof(i32)) < (vscale x 4 x sizeof(i32))
  store i32 %something, i32* %gep
  %gep2 = getelementptr i32, i32* %gep, i64 2   ; now a stack protector is needed because
                                                ; it exceeds `sizeof(4 x i32) - 2 x sizeof(i32)`
  store i32 %something, i32* %gep2


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18431
+  MachineFrameInfo &MFI = MF.getFrameInfo();
+  // If we have any vulnerable SVE stack objects then the stack protector
+  // needs to be placed at the top of the SVE stack area, as the SVE locals
----------------
Is it worth adding a FIXME saying that it would be nice to have this in a new interface, something like `getStackIDForStackProtectorIndex`, rather than doing it in finalizeLowering?


================
Comment at: llvm/test/CodeGen/AArch64/stack-guard-reassign-sve.mir:20
+# should end up in the SVE stack area with 0 (the stack guard) on top.
+  - { id: 0, name: StackGuardSlot, size: 8, alignment: 16, stack-id: scalable-vector }
+# CHECK:       - { id: 0, name: StackGuardSlot, type: default, offset: -16, size: 8,
----------------
nit: It would be better to reorder these objects so that the StackGuardSlot is not the first object, so that you can test it still gets allocated at the top of the frame. (because the algorithm processes the objects in order of `id`)


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

https://reviews.llvm.org/D111631