[X86] Use precise CFI for pushes by default

[mkuper]

Summary

As discussed on the list, this always generates precise CFI.

I'd still like to hook up the imprecise behavior to a switch, but I'm not sure exactly what kind of switch. So I'd like to leave the other option in, but temporary dead. If there's a strong objection to that, I'll pull it out completely, and re-introduce later.

[mkuper]

test/CodeGen/X86/push-cfi.ll

1 ↗

(On Diff #41032)

Ignore this, should not have the new prefix here.

[rnk]

MachO compact unwind info can't support precise unwind info, though, right? Should this depend on the target?

[DavidKreitzer]

Just a few minor comments, Michael. Otherwise, I think this looks good.

-Dave

include/llvm/CodeGen/MachineModuleInfo.h

170 ↗

(On Diff #41032)

It might be a good idea to add the main points of our discussion to this comment:

• that synchronous (i.e. non-precise) unwind info should be enabled under some yet-to-be-defined switch
• that we would first need the ability to simultaneously emit a synchronous .eh_frame and precise .debug_frame under that switch

243 ↗

(On Diff #41032)

This should return void, right?

test/CodeGen/X86/push-cfi.ll

12 ↗

(On Diff #41032)

typo "eeither"

[mkuper]

Thank Dave, Reid!

In D14948#296095, @rnk wrote:

MachO compact unwind info can't support precise unwind info, though, right? Should this depend on the target?

Right. but it can't support the "less precise" version either, if there is more than one call in a function. So this doesn't control the MachO behavior.

include/llvm/CodeGen/MachineModuleInfo.h

170 ↗

(On Diff #41032)

Yes, right, no point in deferring right until I actually add the switch.

243 ↗

(On Diff #41032)

Argh, right, thanks!

[mkuper]

Addressed Dave's comments.

[mkuper]

ping

[• rafael]

include/llvm/CodeGen/MachineModuleInfo.h

168 ↗

(On Diff #41245)

Don't repeat name in comment.

249 ↗

(On Diff #41245)

Nothing calls this, so it is dead code.

Do you have a followup patch that uses it?

[mkuper]

Thanks, Rafael.

include/llvm/CodeGen/MachineModuleInfo.h

168 ↗

(On Diff #41245)

Following style of surrounding code...

249 ↗

(On Diff #41245)

Not yet, although one is eventually planned, per the comment in line 176.
As I wrote in the summary, I can pull out the dead code (and replace it with TODOs), if there's an objection to keeping it in the meanwhile.

[mkuper]

Replaced dead code with TODOs to bring it back.

[• rafael]

LGTM

thanks

llvm/trunk/include/llvm/CodeGen/MachineModuleInfo.h

class MachineModuleInfo : public ImmutablePass {
   /// AddrLabelSymbols - This map keeps track of which symbol is being used for
   /// the specified basic block's address of label.
   MMIAddrLabelMap *AddrLabelSymbols;
 
   bool CallsEHReturn;
   bool CallsUnwindInit;
   bool HasEHFunclets;
 
+  // TODO: Ideally, what we'd like is to have a switch that allows emitting 
+  // synchronous (precise at call-sites only) CFA into .eh_frame. However,
+  // even under this switch, we'd like .debug_frame to be precise when using.
+  // -g. At this moment, there's no way to specify that some CFI directives
+  // go into .eh_frame only, while others go into .debug_frame only.
+
   /// DbgInfoAvailable - True if debugging information is available
   /// in this module.
   bool DbgInfoAvailable;
 
   /// UsesVAFloatArgument - True if this module calls VarArg function with
   /// floating-point arguments.  This is used to emit an undefined reference
   /// to _fltused on Windows targets.
   bool UsesVAFloatArgument;
const Ty &getObjFileInfo() const {
     return const_cast<MachineModuleInfo*>(this)->getObjFileInfo<Ty>();
   }
 
   /// hasDebugInfo - Returns true if valid debug info is present.
   ///
   bool hasDebugInfo() const { return DbgInfoAvailable; }
   void setDebugInfoAvailability(bool avail) { DbgInfoAvailable = avail; }
 
-  // Returns true if we need to generate precise CFI. Currently
-  // this is equivalent to hasDebugInfo(), but if we ever implement
-  // async EH, it will require precise CFI as well.
-  bool usePreciseUnwindInfo() const { return hasDebugInfo(); }
-
   bool callsEHReturn() const { return CallsEHReturn; }
   void setCallsEHReturn(bool b) { CallsEHReturn = b; }
 
   bool callsUnwindInit() const { return CallsUnwindInit; }
   void setCallsUnwindInit(bool b) { CallsUnwindInit = b; }
 
   bool hasEHFunclets() const { return HasEHFunclets; }
   void setHasEHFunclets(bool V) { HasEHFunclets = V; }

llvm/trunk/lib/Target/X86/X86CallFrameOptimization.cpp

if (MOV->getOpcode() == X86::MOV32mi) {
         Push = BuildMI(MBB, Context.Call, DL, TII->get(X86::PUSH32r))
             .addReg(Reg)
             .getInstr();
       }
     }
 
     // For debugging, when using SP-based CFA, we need to adjust the CFA
     // offset after each push.
-    if (!TFL->hasFP(MF) && MF.getMMI().usePreciseUnwindInfo())
+    // TODO: This is needed only if we require precise CFA.
+    if (!TFL->hasFP(MF))
       TFL->BuildCFI(MBB, std::next(Push), DL, 
                     MCCFIInstruction::createAdjustCfaOffset(nullptr, 4));
 
     MBB.erase(MOV);
   }
 
   // The stack-pointer copy is no longer used in the call sequences.
   // There should not be any other users, but we can't commit to that, so:

llvm/trunk/lib/Target/X86/X86FrameLowering.cpp

if (!reserveCallFrame) {
 
     if (Amount == 0)
       return;
 
     // Factor out the amount that gets handled inside the sequence
     // (Pushes of argument for frame setup, callee pops for frame destroy)
     Amount -= InternalAmt;
 
-    // If this is a callee-pop calling convention, and we're emitting precise
-    // SP-based CFI, emit a CFA adjust for the amount the callee popped.
-    if (isDestroy && InternalAmt && DwarfCFI && !hasFP(MF) && 
-        MMI.usePreciseUnwindInfo())
+    // TODO: This is needed only if we require precise CFA.
+    // If this is a callee-pop calling convention, emit a CFA adjust for
+    // the amount the callee popped.
+    if (isDestroy && InternalAmt && DwarfCFI && !hasFP(MF))
       BuildCFI(MBB, I, DL, 
                MCCFIInstruction::createAdjustCfaOffset(nullptr, -InternalAmt));
 
     if (Amount) {
       // Add Amount to SP to destroy a frame, and subtract to setup.
       int Offset = isDestroy ? Amount : -Amount;
 
       if (!(Fn->optForMinSize() && 
             adjustStackWithPops(MBB, I, DL, Offset)))
         BuildStackAdjustment(MBB, I, DL, Offset, /*InEpilogue=*/false);
     }
 
     if (DwarfCFI && !hasFP(MF)) {
       // If we don't have FP, but need to generate unwind information,
       // we need to set the correct CFA offset after the stack adjustment.
       // How much we adjust the CFA offset depends on whether we're emitting
       // CFI only for EH purposes or for debugging. EH only requires the CFA
       // offset to be correct at each call site, while for debugging we want
       // it to be more precise.
       int CFAOffset = Amount;
-      if (!MMI.usePreciseUnwindInfo())
-        CFAOffset += InternalAmt;
+      // TODO: When not using precise CFA, we also need to adjust for the
+      // InternalAmt here.
+
+      if (CFAOffset) {
         CFAOffset = isDestroy ? -CFAOffset : CFAOffset;
         BuildCFI(MBB, I, DL, 
                  MCCFIInstruction::createAdjustCfaOffset(nullptr, CFAOffset));
       }
+    }
 
     return;
   }
 
   if (isDestroy && InternalAmt) {
     // If we are performing frame pointer elimination and if the callee pops
     // something off the stack pointer, add it back.  We do this until we have
     // more advanced stack pointer tracking ability.

llvm/trunk/lib/Target/X86/X86MCInstLower.cpp

case X86::MOVPC32r: {
     // FIXME: We would like an efficient form for this, so we don't have to do a
     // lot of extra uniquing.
     EmitAndCountInstruction(MCInstBuilder(X86::CALLpcrel32)
       .addExpr(MCSymbolRefExpr::create(PICBase, OutContext)));
 
     const X86FrameLowering* FrameLowering =
         MF->getSubtarget<X86Subtarget>().getFrameLowering();
     bool hasFP = FrameLowering->hasFP(*MF);
     
-    bool NeedsDwarfCFI = MMI->usePreciseUnwindInfo();
+    // TODO: This is needed only if we require precise CFA.
+    bool NeedsDwarfCFI = 
+         (MMI->hasDebugInfo() || MF->getFunction()->needsUnwindTableEntry());
     int stackGrowth = -RI->getSlotSize();
 
     if (NeedsDwarfCFI && !hasFP) {
       OutStreamer->EmitCFIAdjustCfaOffset(-stackGrowth);
     }
 
     // Emit the label.
     OutStreamer->EmitLabel(PICBase);

llvm/trunk/test/CodeGen/X86/push-cfi.ll

 ; RUN: llc < %s -mtriple=i686-pc-linux | FileCheck %s -check-prefix=LINUX -check-prefix=CHECK
 ; RUN: llc < %s -mtriple=i686-apple-darwin | FileCheck %s -check-prefix=DARWIN -check-prefix=CHECK
 
 declare i32 @__gxx_personality_v0(...)
 declare void @good(i32 %a, i32 %b, i32 %c, i32 %d)
 declare void @large(i32 %a, i32 %b, i32 %c, i32 %d, i32 %e, i32 %f)
 declare void @empty()
 
-; When we use an invoke, and have FP, we expect a .cfi_escape GNU_ARGS_SIZE
-; with size 16 before the invocation. Without FP, we expect.cfi_adjust_cfa_offset
-; before and after.
-; Darwin should not generate pushes in neither circumstance.
+; When we use an invoke, we expect a .cfi_escape GNU_ARGS_SIZE
+; with size 16 before the invocation. Without FP, we also expect
+; .cfi_adjust_cfa_offset after each push.
+; Darwin should not generate pushes in either circumstance.
 ; CHECK-LABEL: test1_nofp:
 ; LINUX: .cfi_escape 0x2e, 0x10
-; LINUX: .cfi_adjust_cfa_offset 16
 ; LINUX-NEXT: pushl   $4
+; LINUX-NEXT: Ltmp{{[0-9]+}}:
+; LINUX-NEXT: .cfi_adjust_cfa_offset 4
 ; LINUX-NEXT: pushl   $3
+; LINUX-NEXT: Ltmp{{[0-9]+}}:
+; LINUX-NEXT: .cfi_adjust_cfa_offset 4
 ; LINUX-NEXT: pushl   $2
+; LINUX-NEXT: Ltmp{{[0-9]+}}:
+; LINUX-NEXT: .cfi_adjust_cfa_offset 4
 ; LINUX-NEXT: pushl   $1
+; LINUX-NEXT: Ltmp{{[0-9]+}}:
+; LINUX-NEXT: .cfi_adjust_cfa_offset 4
 ; LINUX-NEXT: call
 ; LINUX-NEXT: addl $16, %esp
 ; LINUX: .cfi_adjust_cfa_offset -16
 ; DARWIN-NOT: .cfi_escape
 ; DARWIN-NOT: pushl
 define void @test1_nofp() #0 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 entry:
   invoke void @good(i32 1, i32 2, i32 3, i32 4)
cleanup:
   ret void
 }
 
 ; If the function has no handlers, we don't need to generate GNU_ARGS_SIZE,
 ; even if it has an unwind table. Without FP, we still need cfi_adjust_cfa_offset,
 ; so darwin should not generate pushes.
 ; CHECK-LABEL: test2_nofp:
 ; LINUX-NOT: .cfi_escape
-; LINUX: .cfi_adjust_cfa_offset 16
-; LINUX-NEXT: pushl   $4
+; LINUX: pushl   $4
+; LINUX-NEXT: Ltmp{{[0-9]+}}:
+; LINUX-NEXT: .cfi_adjust_cfa_offset 4
 ; LINUX-NEXT: pushl   $3
+; LINUX-NEXT: Ltmp{{[0-9]+}}:
+; LINUX-NEXT: .cfi_adjust_cfa_offset 4
 ; LINUX-NEXT: pushl   $2
+; LINUX-NEXT: Ltmp{{[0-9]+}}:
+; LINUX-NEXT: .cfi_adjust_cfa_offset 4
 ; LINUX-NEXT: pushl   $1
+; LINUX-NEXT: Ltmp{{[0-9]+}}:
+; LINUX-NEXT: .cfi_adjust_cfa_offset 4
 ; LINUX-NEXT: call
 ; LINUX-NEXT: addl $16, %esp
 ; LINUX: .cfi_adjust_cfa_offset -16
 ; DARWIN-NOT: .cfi_escape
 ; DARWIN-NOT: pushl
 define void @test2_nofp() #0 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 entry:
   call void @good(i32 1, i32 2, i32 3, i32 4)
landingpad { i8*, i32 }
      cleanup
   ret void
 }
 
 ; If we did use pushes, we need to reset GNU_ARGS_SIZE before a call
 ; without parameters, but don't need to adjust the cfa offset
 ; CHECK-LABEL: test5_nofp:
 ; LINUX: .cfi_escape 0x2e, 0x10
-; LINUX: .cfi_adjust_cfa_offset 16
 ; LINUX-NEXT: pushl   $4
+; LINUX-NEXT: Ltmp{{[0-9]+}}:
+; LINUX-NEXT: .cfi_adjust_cfa_offset 4
 ; LINUX-NEXT: pushl   $3
+; LINUX-NEXT: Ltmp{{[0-9]+}}:
+; LINUX-NEXT: .cfi_adjust_cfa_offset 4
 ; LINUX-NEXT: pushl   $2
+; LINUX-NEXT: Ltmp{{[0-9]+}}:
+; LINUX-NEXT: .cfi_adjust_cfa_offset 4
 ; LINUX-NEXT: pushl   $1
+; LINUX-NEXT: Ltmp{{[0-9]+}}:
+; LINUX-NEXT: .cfi_adjust_cfa_offset 4
 ; LINUX-NEXT: call
 ; LINUX-NEXT: addl $16, %esp
 ; LINUX: .cfi_adjust_cfa_offset -16
 ; LINUX-NOT: .cfi_adjust_cfa_offset
 ; LINUX: .cfi_escape 0x2e, 0x00
 ; LINUX-NOT: .cfi_adjust_cfa_offset
 ; LINUX: call
 define void @test5_nofp() #0 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {

llvm/trunk/test/CodeGen/X86/tls-pie.ll

 
 entry:
   ret i32* @i
 }
 
 define i32 @f3() {
 ; X32-LABEL: f3:
 ; X32:      calll .L{{[0-9]+}}$pb
+; X32-NEXT: .Ltmp{{[0-9]+}}:
+; X32-NEXT: .cfi_adjust_cfa_offset 4
 ; X32-NEXT: .L{{[0-9]+}}$pb:
 ; X32-NEXT: popl %eax
 ; X32-NEXT: .Ltmp{{[0-9]+}}:
+; X32-NEXT: .cfi_adjust_cfa_offset -4
+; X32-NEXT: .Ltmp{{[0-9]+}}:
 ; X32-NEXT: addl $_GLOBAL_OFFSET_TABLE_+(.Ltmp{{[0-9]+}}-.L{{[0-9]+}}$pb), %eax
 ; X32-NEXT: movl i2@GOTNTPOFF(%eax), %eax
 ; X32-NEXT: movl %gs:(%eax), %eax
 ; X32-NEXT: ret
 ; X64-LABEL: f3:
 ; X64:      movq i2@GOTTPOFF(%rip), %rax
 ; X64-NEXT: movl %fs:(%rax), %eax
 ; X64-NEXT: ret
 
 entry:
   %tmp1 = load i32, i32* @i2
   ret i32 %tmp1
 }
 
 define i32* @f4() {
 ; X32-LABEL: f4:
 ; X32:      calll .L{{[0-9]+}}$pb
+; X32-NEXT: .Ltmp{{[0-9]+}}:
+; X32-NEXT: .cfi_adjust_cfa_offset 4
 ; X32-NEXT: .L{{[0-9]+}}$pb:
 ; X32-NEXT: popl %ecx
 ; X32-NEXT: .Ltmp{{[0-9]+}}:
+; X32-NEXT: .cfi_adjust_cfa_offset -4
+; X32-NEXT: .Ltmp{{[0-9]+}}:
 ; X32-NEXT: addl $_GLOBAL_OFFSET_TABLE_+(.Ltmp{{[0-9]+}}-.L{{[0-9]+}}$pb), %ecx
 ; X32-NEXT: movl %gs:0, %eax
 ; X32-NEXT: addl i2@GOTNTPOFF(%ecx), %eax
 ; X32-NEXT: ret
 ; X64-LABEL: f4:
 ; X64:      movq %fs:0, %rax
 ; X64-NEXT: addq i2@GOTTPOFF(%rip), %rax
 ; X64-NEXT: ret
 
 entry:
   ret i32* @i2
 }

llvm/trunk/test/CodeGen/X86/win32-pic-jumptable.ll

 ; RUN: llc < %s -relocation-model=pic | FileCheck %s
 
 ; CHECK:        calll L0$pb
+; CHECK-NEXT: Ltmp{{[0-9]+}}:
+; CHECK-NEXT: .cfi_adjust_cfa_offset 4
 ; CHECK-NEXT: L0$pb:
 ; CHECK-NEXT:   popl %eax
+; CHECK-NEXT: Ltmp{{[0-9]+}}:
+; CHECK-NEXT: .cfi_adjust_cfa_offset -4
 ; CHECK-NEXT:   addl LJTI0_0(,%ecx,4), %eax
 ; CHECK-NEXT:   jmpl *%eax
 
 ; CHECK:      LJTI0_0:
 ; CHECK-NEXT:   .long LBB0_2-L0$pb
 ; CHECK-NEXT:   .long LBB0_3-L0$pb
 ; CHECK-NEXT:   .long LBB0_4-L0$pb
 ; CHECK-NEXT:   .long LBB0_5-L0$pb