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

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas marked 2 inline comments as done.
cdevadas added inline comments.


================
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.
----------------
scott.linder wrote:
> 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.
I don't think we need to stress it out for the FP. 
This is a usual comment for BP in the frame lowering file for each target. Sure, 'locals' here is misleading. I will change it to function arguments.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:118
+  // stack pointer, so we reserve a base pointer.
+  return needsStackRealignment(MF);
+}
----------------
scott.linder wrote:
> 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.
It sounds good to me. We can enable it only when there is a fixed-frame. 
It can be easily done with an additional check, MFI.getNumFixedObjects() != 0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78811