[PATCH] D81364: AMDGPU: Correct prolog SP initialization logic

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1265
 
+/// Returns true if the frame will require a reference to the stack pointer.
+/// FIXME: Should also check hasOpaqueSPAdjustment and if any inline asm
----------------
So this is the function for "the conditions which imply the need for setting up an SP which are shared for entry functions and non-entry functions; just checking this for a frame can give false-negatives"?

I think at first I missed that this is just a `static` function used to implement the other bits and was confused. I don't know if it actually would help, but maybe you could hint at this in the name with something like `frameTriviallyRequiresSP`, and note in the comment that this is just the conditions shared between all frames?


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1295
+
+bool SIFrameLowering::requiresStackPointerReference(const MachineFunction &MF) const {
+  // Callable functions always require a stack pointer reference.
----------------
arsenm wrote:
> scott.linder wrote:
> > How does this differ from `frameRequiresSP`?
> This is split mostly because of the distinctive handling of functions and kernels in hasFP. hasFP and requiresStackPointerReferences are almost the same, except the kernel case is a bit more specific since there's never a real reason to setup FP since it's known 0 (e.g. we never realign the kernel stack). requiresStackPointerReference is essentially hasFP for kernels, and frameRequiresSP is common to kernels and functions.
Can you put a (possibly condensed) version of this in a comment on the function? I mistakenly thought the `frameRequiresSP` was public, so the comment doesn't need to actually reference it, but just stating what the function does would help with the ambiguity between all of the `needs{S,F}P` type functions.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1297
+  // Callable functions always require a stack pointer reference.
+  if (!MF.getInfo<SIMachineFunctionInfo>()->isEntryFunction())
+    return true;
----------------
Can this just be an assert that it is called for an entry function, and then could the name of this function just include `EntryFunction` like e.g. `emitEntryFunctionPrologue` does?


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

https://reviews.llvm.org/D81364