[PATCH] D74303: [CFI] cfi directive insertion for callee-save-registers in CFIInstrInserter pass

[Wei Mi via Phabricator via llvm-commits]

wmi marked 2 inline comments as done.
wmi added a comment.

In D74303#1870079 <https://reviews.llvm.org/D74303#1870079>, @thegameg wrote:

> Thanks for working on this, I think in general it looks good.
>
> Wasn't there a verifier that was running with EXPENSIVE_CHECKS for the previously supported directives? Can we add the registers as well?
>
> More comments inline.


Thanks for the review and sorry for the late reply.

And thanks for pointing out the verifier. That is a helpful suggestion! I added the check for CSR related CFI, and it helped me to discover a problem I missed before: Previously we skip inserting .cfi_restore in epilogue in prologue/epilogue generation pass, and only insert .cfi_restore in CFIInstrInserter pass. If the epilogue block is not a return block, skipping inserting .cfi_restore in it will lead to the CSR related CFI verification error -- the return block after the epilogue block will see different OutgoingCSRSaved set from its predecessors.

The solution is: In prologue/epilogue generation pass, we need to insert .cfi_restore for epilogue if it is not a return block or other type of exit block. In this way, each block will see consistent OutgoingCSRSaved information from each of its predecessors. At the same time, for the common case that epilogue block is a return block, we still don't need to insert unnecessary .cfi_restore.

To make it clearer, here is a testcase to show it. In the assembly, %bb.1 is an epilogue bb without return inside of it. If no .cfi_restore is inserted in %bb.1, %LBB0_2 will see different OutgoingCSRSaved information from its two predecessors: %bb.0 (no reg saved) and %bb.1 (rbp saved). So we need to insert .cfi_restore in %bb.1.

  ------------- 1.cc --------------
  void goo();
  
  long foo(bool cond) {
    if (__builtin_expect(cond, 1)) {
      goo();
      return 0;
    }
    return 0;
  }
  -----------------------------
  
  clang -O2 -fno-omit-frame-pointer  -S 1.cc
    .cfi_startproc
  # %bb.0:                                # %entry
    testb %dil, %dil
    je  .LBB0_2
  # %bb.1:                                # %if.then
    pushq %rbp
    .cfi_def_cfa_offset 16
    .cfi_offset %rbp, -16
    movq  %rsp, %rbp
    .cfi_def_cfa_register %rbp
    callq _Z3goov
    popq  %rbp                     
    .cfi_def_cfa %rsp, 8
  .LBB0_2:                                # %return
    xorl  %eax, %eax
    retq
  .Lfunc_end0:
    .size _Z3foob, .Lfunc_end0-_Z3foob
    .cfi_endproc





================
Comment at: llvm/lib/CodeGen/CFIInstrInserter.cpp:81
+    /// Set of callee saved registers saved at basic block entry.
+    std::set<unsigned> IncomingCSRSaved;
+    /// Set of callee saved registers saved at basic block exit.
----------------
thegameg wrote:
> You can use `BitVector`s as sets of registers, with the supported bitwise operations to provide similar set operations as std::set.
Thanks. Done.


================
Comment at: llvm/test/CodeGen/X86/cfi-inserter-callee-save-register.mir:6
+--- |
+  define void @_Z3foobPl(i64* nocapture readonly %a) {
+  entry:
----------------
thegameg wrote:
> Does this test actually need the IR?
The function define is needed, but its body can be further simplified. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D74303