[PATCH] D96006: [AArch64] Stack probing for dynamic allocas in GlobalISel

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

Seems fine from a GlobalISel perspective, and I'm guessing the stack clashing details will be reviewed in the SelectionDAG patch which adds the test.



================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:681
+
+  getActionDefinitionsBuilder(G_DYN_STACKALLOC).customFor({{p0, s64}}).lower();
 
----------------
Why is this custom only for {p0, s64} ? It seems all the existing tests for G_DYN_STACKALLOC are for {p0, s64}, so I'm not sure when the other case is even possible and why we can't use the probing then too.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp:765
+  SPTmp = MIRBuilder.buildCast(PtrTy, Alloc);
+  auto NewMI =
+      MIRBuilder.buildInstr(AArch64::PROBED_STACKALLOC_DYN, {}, {SPTmp});
----------------
Would it be a good idea to share some of the code between this handler and lowerDynStackAlloc? Maybe add a method to LegalizeHelper that does the allocation and returns the SPTmp. , then the code here would be just
/* check if stack probing is enabled */
SPTmp = Helper.allocateStack /* (or some other suggestive name) */
/* generate PROBED_STACKALLOC_DYN etc */


================
Comment at: llvm/test/CodeGen/AArch64/stack-probing-dynamic.ll:3
+; RUN: llc -mtriple aarch64-none-eabi < %s -verify-machineinstrs              | FileCheck %s
+; RUN: llc -mtriple aarch64-none-eabi < %s -verify-machineinstrs -global-isel | FileCheck %s
 
----------------
I love that this test has the same checks for GlobalISel and SelectionDAG :)

Nitpick: I think it would be nice to also test this in llvm/test/CodeGen/AArch64/GlobalISel/legalize-dyn-alloca.mir, which covers just the legalizer.


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

https://reviews.llvm.org/D96006