[PATCH] D79978: Call Frame Information (CFI) Handling for Basic Block Sections

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfException.h:70-71
+
+  void beginBasicBlock(const MachineBasicBlock &MBB) override;
+  void endBasicBlock(const MachineBasicBlock &MBB) override;
 };
----------------
They are new functions marked as override, but there are no virtual functions added in the base class.  


================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:316-322
+    if (PrevMBBInfo->OutgoingCFAOffset != MBBInfo.IncomingCFAOffset ||
+        ForceFullCFA) {
       // If both outgoing offset and register of a previous block don't match
       // incoming offset and register of this block, add a def_cfa instruction
       // with the correct offset and register for this block.
-      if (PrevMBBInfo->OutgoingCFARegister != MBBInfo.IncomingCFARegister) {
+      if (PrevMBBInfo->OutgoingCFARegister != MBBInfo.IncomingCFARegister ||
+          ForceFullCFA) {
----------------
Can we refactor it a little so the case to generate createDefCfa can be separated out like following? It is easier to understand when ForceFullCFA is true, only createDefCfa will be used to set up CFA info.

```
if (PrevMBBInfo->OutgoingCFAOffset != MBBInfo.IncomingCFAOffset &&
    PrevMBBInfo->OutgoingCFARegister != MBBInfo.IncomingCFARegister || ForceFullCFA) {
  MF.addFrameInst(MCCFIInstruction::createDefCfa...
  ...
} else if (...) {
...
```


================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:381-383
+    if (ForceFullCFA) {
+      MF.getSubtarget().getFrameLowering()->emitCalleeSavedFrameMoves(
+          *MBBInfo.MBB, MBBI);
----------------
Better move the snippet upwards and early return for ForceFullCFA in case later people add continue before the snippet since it is expected to call emitCalleeSavedFrameMoves for every BB except the first BB.


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

https://reviews.llvm.org/D79978