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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

This is starting to look good, a few more nitpicks below and hopefullly @iteratee will comment on the isDirective() thing.



================
Comment at: include/llvm/CodeGen/MachineInstr.h:795
 
+  bool isDirective() const { return isDebugValue() || isCFIInstruction(); }
   bool isPHI() const {
----------------
violetav wrote:
> MatzeB wrote:
> > This name is unintuitive to me. And it feels like this isn't really a category of machine instructions but rather just a combination that `BranchFolding` is able to deal with after this patch.
> > 
> > Maybe it would be better to move this into BranchFolding.cpp then in the form of
> > `static bool isDirective(const MachineInstr &MI) { return MI.isDebugValue() || MI.isCFIInstruction(); }`.
> This is used in BranchFolding and TailDuplication. It was suggested previously to be done this way https://reviews.llvm.org/D18046?id=101198#inline-294697
@iteratee Have you seen isMetaInstruction() and isTransient()? This feels like yet another variation of machine instructions that don't produce code. However I don't know why this specific combination is interesting and the generic name and no comment doesn't help understanding that either.


================
Comment at: lib/CodeGen/MachineInstr.cpp:333-353
+      case MCCFIInstruction::OpDefCfa:
+      case MCCFIInstruction::OpOffset:
+      case MCCFIInstruction::OpRelOffset:
+        if (Inst.getRegister() != OtherInst.getRegister()) return false;
+        if (Inst.getOffset() != OtherInst.getOffset()) return false;
+        break;
+      case MCCFIInstruction::OpRestore:
----------------
Keep `case`/`default` at the same indentation level as the switch.


================
Comment at: lib/CodeGen/MachineInstr.cpp:354-355
+        break;
+      default:
+        break;
+    }
----------------
As there are only 2 or 3 cases that are not handled yet, I would recommend to explicitely add `case`s for them and not add a `default:` at all. The benefit of this is that when someone adds new `OpType`s in the future he will immediately get a compiler warning for a missing case when he forgets to add a case here.


================
Comment at: lib/CodeGen/TargetFrameLoweringImpl.cpp:110
+  llvm_unreachable("getInitialCFAOffset() not implemented!");
+  return -1;
+}
----------------
llvm_unreachable never returns so you can leave out the `return` statement behind it. Same in the next function.


Repository:
  rL LLVM

https://reviews.llvm.org/D35844