[PATCH] D132436: [AMDGPU][SIFrameLowering] Unify custom SGPR spill saves and restores (NFC)

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:31
+// throughout the function. On failure, returns a null register.
+static MCRegister findUnusedScratchNonCSR(MachineRegisterInfo &MRI,
+                                          LivePhysRegs &LiveRegs,
----------------
sebastian-ne wrote:
> This function doesn’t do anything with CSRs, although the name says so?
You're right. Neither scratch nor CSR should be in the name. I will change it.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:100
     } else {
       // Remove dead <NewFI> index
+      MF.getFrameInfo().RemoveStackObject(FI);
----------------
sebastian-ne wrote:
> Comment should say <FI> for the renamed variable.
Thanks. Will change it.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:269
+
+  void copyToScratchSGPR(Register DstReg) const {
+    const TargetRegisterClass *RC = TRI.getPhysRegClass(DstReg);
----------------
sebastian-ne wrote:
> Why does this code split the copies into SubRegs and copyFromScratchSGPR doesn’t?
For the purpose of emitting CFI directives for each copy. There is no CFI emitted in the epilogue.
There is more coming in the downstream version of this patch.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.h:37-38
+// at function prolog/epilog.
+namespace RegSaveInfo {
+enum RegSaveKind : uint8_t {
+  COPY_TO_SCRATCH_SGPR,
----------------
sebastian-ne wrote:
> Why not make this an `enum class RegSaveKind`?
Will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132436