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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D79978#2145413 <https://reviews.llvm.org/D79978#2145413>, @tmsriram wrote:

> ...
>  Now, what happens when we start placing individual basic blocks in unique sections:
>
> - Basic block sections allow the linker to randomly reorder basic blocks in the address space such that a given basic block can become non-contiguous with the original function.
> - The different basic block sections can no longer share the cfi_startproc and cfi_endproc directives.  So, each basic block section should emit this independently.
> - Each (cfi_startproc, cfi_endproc) directive will result in a new FDE that caters to that basic block section.
> - Now, this basic block section needs to duplicate the information from the entry block to compute the CFA as it is an independent entity.  It cannot refer to the FDE of the original function and hence must duplicate all the stuff that is needed to compute the CFA on its own. ...


Thanks for the detailed write-up! I I've learned a bit from it. I think at least the quoted items should be placed in the description.

In D79978#2145189 <https://reviews.llvm.org/D79978#2145189>, @amharc wrote:

> > Note also that only `rbp` is described. I think we need another register to demonstrate the effect.
>
> `rbp` is the usual frame pointer register for the x86 architecture and I'm not really sure we can easily force the compiler to choose a different register to hold the frame pointer. If you know how to force a different register to be the frame pointer, please let us know - we will add a corresponding test.


I cannot find a test with `.cfa_offset` or describing a non-rbp register, so the implementation is under-tested. How about leveraging inline asm to clobber callee-saved registered?

  int f3(int i, int j, int k) {
    if (i == 0) { // adds a basic block
      asm("nop" : : : "rdi","rsi","rdx","rbp","r12","r13","r14","r15","memory"); // there is a .cfi_offset for each of rbp,r12,r13,r14,r15
      return j;
    }
    if (j == 0) {
      asm("xchg %%ax,%%ax" : : : "rdi","rsi","rdx","rbp","r14","r15","memory");  // r12 and r13 are not clobbered but the current implementation adds .cfi_offset for both r12 and r13
      return k;
    }
    return i;
  }

I get a lot of `.cfi_offset` with `clang -S -emit-llvm -O1 a.c; llc -O0 -basicblock-sections=all < a.ll`:

  ...
          .section        .text,"ax", at progbits,unique,1
  f3.1:                                   # %if.then
          .cfi_startproc
          .cfi_def_cfa %rsp, 48
          .cfi_offset %r12, -48
          .cfi_offset %r13, -40
          .cfi_offset %r14, -32
          .cfi_offset %r15, -24
          .cfi_offset %rbp, -16
          #APP
          nop
          #NO_APP
          movl    -8(%rsp), %eax                  # 4-byte Reload
          movl    %eax, -16(%rsp)                 # 4-byte Spill
  ...



================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:484
+/// frame pointer.
+void X86FrameLowering::emitCalleeSavedFrameMoves(
+    MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI) const {
----------------
If the function is only used by basic block sections, please mention the fact.


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

https://reviews.llvm.org/D79978