[PATCH] D20220: [PEI, AArch64] Use empty spaces in stack area for local stack slot allocation.

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:542
@@ +541,3 @@
+  if (StackBytesFree.all()) {
+    StackBytesFree.resize(0);
+    return false;
----------------
resize(0) is weird when reading the code. Clear() looks more appropriate.

Also, if all the bytes are free, why do we stop?

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:550
@@ +549,3 @@
+
+  auto ObjSize = MFI->getObjectSize(FrameIdx);
+  int FreeStart;
----------------
use the actual type here.
It is important to know if it is a int an unsigned, etc.

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:680
@@ -618,2 +679,3 @@
 
+  int64_t FixedCSOffset = Offset;
   unsigned MaxAlign = MFI->getMaxAlignment();
----------------
Add a comment on what information we keep in FixedCSOffset, e.g., offset containing all the fixed sized and callee saved objects.

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:815
@@ +814,3 @@
+  // can use the holes when allocating later stack objects.  Only do this if
+  // stack protecter isn't being used the target requests it.
+  BitVector StackBytesFree;
----------------
protecter -> protector

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:815
@@ +814,3 @@
+  // can use the holes when allocating later stack objects.  Only do this if
+  // stack protecter isn't being used the target requests it.
+  BitVector StackBytesFree;
----------------
qcolombet wrote:
> protecter -> protector
isn’t being used *and* the target...

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:818
@@ +817,3 @@
+  if (!ObjectsToAllocate.empty() &&
+      MFI->getStackProtectorIndex() < 0 && TFI.enableStackSlotScavenging(Fn)) {
+    StackBytesFree.resize(FixedCSOffset, true);
----------------
We should gate that with the optimization level as well.

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:821
@@ +820,3 @@
+
+    SmallVector<int, 16> AllocedFrameSlots;
+    // Fixed objects
----------------
Allocated

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:822
@@ +821,3 @@
+    SmallVector<int, 16> AllocedFrameSlots;
+    // Fixed objects
+    for (int i = MFI->getObjectIndexBegin(); i != 0; ++i)
----------------
Period.

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:825
@@ +824,3 @@
+      AllocedFrameSlots.push_back(i);
+    // Callee-save objects
+    for (int i = MinCSFrameIndex; i <= (int)MaxCSFrameIndex; ++i)
----------------
Period.

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:834
@@ +833,3 @@
+      if (StackGrowsDown) {
+        ObjStart = -ObjOffset - ObjSize;
+        ObjEnd = -ObjOffset;
----------------
A comment reminding that ObjOffset is negative would be nice :).

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:837
@@ +836,3 @@
+      } else {
+        ObjSize = ObjOffset;
+        ObjEnd = ObjOffset + ObjSize;
----------------
ObjStart

================
Comment at: lib/CodeGen/PrologEpilogInserter.cpp:844
@@ +843,3 @@
+    }
+  }
+
----------------
Could you move all that stuff in a helper function?

================
Comment at: test/CodeGen/AArch64/aarch64-dynamic-stack-layout.ll:681
@@ -680,3 +680,3 @@
 bb0:
-  %MyAlloca = alloca i8, i64 64, align 32
+  %MyAlloca = alloca i8, i64 64, align 64
   br label %bb1
----------------
Why is the input of the test case changed?


http://reviews.llvm.org/D20220