[PATCH] D95241: AMDGPU: Fix redundant FP spilling/assert in some functions

Matt Arsenault via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 22 12:20:04 PST 2021


arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1349
+  // FP will be specially managed like SP.
+  if (WillHaveFP || hasFP(MF))
+    SavedRegs.reset(MFI->getFrameOffsetReg());
----------------
scott.linder wrote:
> Is there a reason to have `hasFP` as distinct from `WillHaveFP`? Is it not possible to have a single predicate which encompasses both? Is it an issue of needing to consider different context for each callsite?
> 
> It seems like the only question any part of the code should be asking is: "Can we prove we do not //and// will not need a frame pointer?" I would expect it to be a "monotone" predicate, in that it can only ever "increase" in strength from "we may need an FP, so you must assume we do/will" to "we absolutely do not need an FP, and will never accidentally end up in a situation later in compilation where we actually do need an FP", and for debug builds this should be enforced via an assertion. It would be safe to implement the predicate to always return "we may need it", but we would want to have it detect as many cases where we could promise no need for an FP starting as early in compilation as possible.
> 
> If I understand the exact issue being addressed here, this patch makes sense, but it feels like we will be hitting other similar issues until we do something more fundamental.
Ultimately this is because our hasFP condition is weird. Unlike every other target, the stack addressing and stack growth are in the same direction so we end up requiring an FP to access stack objects if there's a call. We then don't know the set of stack objects because the callee saved spills are inserted later.

Once we start making decisions that assume hasFP, we're stuck with the FP.  We already do the same prediction in determineCalleeSaves, this just repeats it in determineCalleeSavesSGPR. This doesn't have to do with the callsites, it's a function of the function with a call. I think I tried before to make hasFP conservative, but I don't remember what the problem was


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

https://reviews.llvm.org/D95241



More information about the llvm-commits mailing list