[PATCH] D76880: [AMDGPU] Emit entry function CFI

[Scott Linder via Phabricator via llvm-commits]

scott.linder marked 3 inline comments as done.
scott.linder added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:394
+                                        // should be defined elsewhere
+      static_cast<char>(unsigned(dwarf::DW_OP_LLVM_form_aspace_address))};
+  BuildCFI(MBB, I, DL,
----------------
arsenm wrote:
> Why two levels of casting in all of these places? I also really don't like the constructor form of casts
Sorry, this is me copy-pasting from the only other use of createEscape in the code base and not thinking about it more. I'll clean all these up.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1074-1085
+void SIFrameLowering::BuildCFI(MachineBasicBlock &MBB,
+                               MachineBasicBlock::iterator MBBI,
+                               const DebugLoc &DL,
+                               const MCCFIInstruction &CFIInst) const {
+  MachineFunction &MF = *MBB.getParent();
+  const GCNSubtarget &ST = MF.getSubtarget<GCNSubtarget>();
+  const SIInstrInfo *TII = ST.getInstrInfo();
----------------
arsenm wrote:
> I don't think this function buys you much since all of these thing are already in scope at the use.
It is more just to cut down on the noise from having to get a CFI index and mark the instruction as FrameSetup. I'm OK inlining it everywhere, but things go from:

```
BuildCFI(MBB, I, DL,
           MCCFIInstruction::createUndefined(
               nullptr, MCRI->getDwarfRegNum(AMDGPU::PC_REG, false)));
```

to

```
BuildCFI(MBB, I, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
       .addCFIIndex(MF.addFrameInst(MCCFIInstruction::createUndefined(
           nullptr, MCRI->getDwarfRegNum(AMDGPU::PC_REG, false))))
       .setMIFlag(MachineInstr::FrameSetup);
```


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.h:78
+  /// Create a CFI index for CFIInst and build a MachineInstr around it.
+  void BuildCFI(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
+                const DebugLoc &DL, const MCCFIInstruction &CFIInst) const;
----------------
arsenm wrote:
> Star with lowercase 
I was trying to mimic BuildMI, which still has the wrong casing, but I'll update all of these.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76880