[PATCH] D51161: Allow inconsistent offsets for 'noreturn' basic blocks when '-verify-cfiinstrs'

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg accepted this revision.
thegameg added a comment.
This revision is now accepted and ready to land.

LGTM, thanks.

Could you also add what you said in the previous comment to the test case?

  C code to reproduce the issue, built with
  
  '-O2 -mllvm -tail-merge-size=1 -mllvm -verify-cfiinstrs':
  
  void foo1(int v) __attribute__((noreturn)) {
    if (v == 1) {
      __builtin_trap();
    }
    if (foo2(v)) {
      __builtin_trap();
    }
  }

Thanks!



================
Comment at: lib/CodeGen/CFIInstrInserter.cpp:320
           SuccMBBInfo.IncomingCFARegister != CurrMBBInfo.OutgoingCFARegister) {
+        // Inconsistent offsets/registers are ok for 'noreturn' blocks.
+        if (SuccMBBInfo.MBB->succ_empty() && !SuccMBBInfo.MBB->isReturnBlock())
----------------
Do you think we should add `because we don't generate epilogues inside noreturn blocks` to the comment? It might make it clearer on why the inconsistency is ok.


================
Comment at: test/CodeGen/X86/cfi-inserter-noreturnblock.mir:7
+--- |
+  define void @testNoreturnBlock() {
+      ret void
----------------
You should be able to remove the LLVM IR completely here.


Repository:
  rL LLVM

https://reviews.llvm.org/D51161