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

[Scott Linder via Phabricator via llvm-commits]

scott.linder 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());
----------------
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.


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

https://reviews.llvm.org/D95241