[PATCH] D35844: Correct dwarf unwind information in function epilogue

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added a comment.

Hi Violeta,

Thanks a lot for working on this! This looks really nice! Seems a lot easier to adapt to other targets and other CFI state like CSRs.



================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:33
+class CFIInstrInserter : public MachineFunctionPass {
+ public:
+  static char ID;
----------------
I think most of the members can be private here.


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:53
+    calculateCFAInfo(MF);
+    verify(MF);
+    if (ErrorNum)
----------------
Should this be guarded by the `EXPENSIVE_CHECKS` macro?


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:62
+
+  typedef struct MBBCFAInfo {
+    MachineBasicBlock *MBB;
----------------
In LLVM we usually use

  struct MBBCFAInfo {
    [...]
  };


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:87
+  // the same.
+  void calculateOutgoingCFAInfo(MBBCFAInfo *MBBInfo);
+  // Update in/out cfa offset and register values for successors of the basic
----------------
Why not a reference instead of a pointer? It can never be null, right?


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:131
+  for (auto &MBB : MF) {
+    struct MBBCFAInfo MBBInfo;
+    MBBInfo.MBB = &MBB;
----------------
No need for `struct` here.


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:156
+  // Outgoing cfa register set by the block.
+  unsigned SetRegister;
+  const std::vector<MCCFIInstruction> &Instrs =
----------------
You can already initialize the variables here.


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:193
+  for (auto Succ : MBBInfo->MBB->successors()) {
+    SuccInfo = &MBBMap[Succ->getNumber()];
+    if (SuccInfo->Processed) continue;
----------------
You could limit the scope of `SuccInfo` to the loop body.


================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:254
+void CFIInstrInserter::report(const char *msg, MachineBasicBlock &MBB) {
+  assert(&MBB);
+  ErrorNum++;
----------------
I think you can assume the reference is always non-null here, right?


Repository:
  rL LLVM

https://reviews.llvm.org/D35844