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

[John Brawn via Phabricator via llvm-commits]

john.brawn added inline comments.


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:969
+               "already set.");
+    } else if (!MFI.isObjectPreAllocated(MFI.getStackProtectorIndex())) {
       llvm_unreachable(
----------------
sdesmalen wrote:
> nit: unnecessary curly braces
That's done for consistency with the preceding if (which has braces because of the nested if/else).


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:970
+    } else if (!MFI.isObjectPreAllocated(MFI.getStackProtectorIndex())) {
       llvm_unreachable(
           "Stack protector not pre-allocated by LocalStackSlotPass.");
----------------
sdesmalen wrote:
> Do we need to some way to ensure that the LocalStackSlotAllocationPass doesn't pre-allocate the stack protector value if we know it will be allocated to a different StackID?
I'll add an assertion to LocalStackSlotAllocationPass to check for that.


================
Comment at: llvm/lib/CodeGen/StackProtector.cpp:214
       // Adjust AllocSize to be the space remaining after this offset.
-      if (HasAddressTaken(I, AllocSize - Offset.getLimitedValue()))
+      TypeSize OffsetSize = TypeSize::Fixed(Offset.getLimitedValue());
+      if (HasAddressTaken(I, AllocSize - OffsetSize))
----------------
sdesmalen wrote:
> If the gep is indexing into a scalable vector type, then OffsetSize cannot be fixed size?
Indexing into a scalable vector depends only on the size of the element type, which is known.


================
Comment at: llvm/lib/CodeGen/StackProtector.cpp:340
         bool IsLarge = false;
-        if (ContainsProtectableArray(AI->getAllocatedType(), IsLarge, Strong)) {
+        Type *AllocType = AI->getAllocatedType();
+        if (ContainsProtectableArray(AllocType, IsLarge, Strong)) {
----------------
sdesmalen wrote:
> unnecessary change?
A relic of the previous version of this patch, I'll remove.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:2960
+    MFI.setObjectAlignment(MFI.getStackProtectorIndex(), Align(16));
+  }
+
----------------
sdesmalen wrote:
> I think this patch also needs some code to `determineSVEStackObjectOffsets` to ensure that the stack protector value is ordered before all other objects when allocating their offsets. (this is probably best tested with an MIR test)
Will do.


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

https://reviews.llvm.org/D111631