[PATCH] D116628: [X86] add dwarf information for loop stack probe

Thu Jan 6 16:40:53 PST 2022

erikdesjardins added inline comments.

================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:715-720
+      BuildCFI(MBB, MBBI, DL,
+               MCCFIInstruction::createDefCfaRegister(
+                   nullptr, TRI->getDwarfRegNum(DwarfFinalStackProbed, true)));
+      BuildCFI(MBB, MBBI, DL,
+               MCCFIInstruction::createAdjustCfaOffset(nullptr, BoundOffset));
+    }
----------------
nagisa wrote:
> 
It's necessary to use `adjust` instead of `def` in case there are any CFA offset changes earlier in the prologue, i.e. due to `push`. Added a test demonstrating this.

It would surely be possible to pass down the current CFA offset through `TargetFrameLowering::inlineStackProbe`, allowing us to use `def` here, but I think it's likely nontrivial for the same reason your other comment is nontrivial.

================
Comment at: llvm/test/CodeGen/X86/stack-clash-large.ll:19-20
 ; CHECK-X64-NEXT:    subq $2248, %rsp # imm = 0x8C8
+; CHECK-X64-NEXT:    .cfi_def_cfa_register %rsp
 ; CHECK-X64-NEXT:    .cfi_def_cfa_offset 71888
 ; CHECK-X64-NEXT:    movl $1, 264(%rsp)
----------------
nagisa wrote:
> In theory this could also be `.cfi_def_cfa %rsp, 71888` but seems like it may be non-trivial to make it happen.
Yep, I had the same thought and came to the same conclusion.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116628