[PATCH] D78811: [AMDGPU] Enable base pointer.

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/docs/AMDGPUUsage.rst:6706
+    to access the incoming stack arguments in the function. The BP is needed
+    only when the function requie the runtime stack alignment.
 
----------------
s/requie/requires/


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:807-810
+  // If we need a base pointer, set it up here. It's whatever the value of
+  // the stack pointer is at this point. Any variable size objects will be
+  // allocated after this, so we can still use the base pointer to reference
+  // locals.
----------------
Is this comment accurate anymore? I think it can go back where it was with "base pointer" just changed to "frame pointer".

A new comment for the "base pointer" would need to talk about referencing function arguments, not locals.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:118
+  // stack pointer, so we reserve a base pointer.
+  return needsStackRealignment(MF);
+}
----------------
Is it possible to have some mechanism for only setting up the BP when it is actually used? Looking at X86 it seems like they don't do this, so I am OK with us not doing it either, just wondering if it is feasible.


================
Comment at: llvm/test/CodeGen/AMDGPU/callee-frame-setup.ll:302
+; GCN-NEXT: s_mov_b32 s5, s34
+; GCN-NEXT: s_mov_b32 s34, s32
 ; GCN-NEXT: s_add_u32 s32, s32, 0x100000
----------------
This is an example of where we have dynamic realignment but don't actually use the BP to refer to a fixed index. Again, being correct is more important, but if it is feasible it seems nice to only set this up when it is used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78811