[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

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

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

This should return void, right?

test/CodeGen/X86/push-cfi.ll

12

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

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

243

Argh, right, thanks!

[mkuper]

Addressed Dave's comments.

[mkuper]

ping

[• rafael]

include/llvm/CodeGen/MachineModuleInfo.h

168

Don't repeat name in comment.

243

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

Following style of surrounding code...

243

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

include/llvm/CodeGen/MachineModuleInfo.h

class MachineModuleInfo : public ImmutablePass {
   bool CallsEHReturn;
   bool CallsUnwindInit;
   bool HasEHFunclets;
 
   /// DbgInfoAvailable - True if debugging information is available
   /// in this module.
   bool DbgInfoAvailable;
 
+  /// UsePreciseUnwindInfo - True if we should emit precise CFA adjustment
+  /// information. False if we only need CFA to be precise at call sites.
+  bool UsePreciseUnwindInfo;
+
   /// 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;
 
   /// UsesMorestackAddr - True if the module calls the __morestack function
   /// indirectly, as is required under the large code model on x86. This is used
   /// to emit a definition of a symbol, __morestack_addr, containing the
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 usePreciseUnwindInfo() const { return UsePreciseUnwindInfo; }
+  bool setUsePreciseUnwindInfo(bool b) { UsePreciseUnwindInfo = b; }
 
   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; }

lib/CodeGen/MachineModuleInfo.cpp

 bool MachineModuleInfo::doInitialization(Module &M) {
 
   ObjFileMMI = nullptr;
   CurCallSite = 0;
   CallsEHReturn = false;
   CallsUnwindInit = false;
   HasEHFunclets = false;
   DbgInfoAvailable = UsesVAFloatArgument = UsesMorestackAddr = false;
+  UsePreciseUnwindInfo = true;
   PersonalityTypeCache = EHPersonality::Unknown;
   AddrLabelSymbols = nullptr;
   TheModule = nullptr;
 
   return false;
 }
 
 bool MachineModuleInfo::doFinalization(Module &M) {

lib/Target/X86/X86FrameLowering.cpp

if (DwarfCFI && !hasFP(MF)) {
       // 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;
+
+      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.

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();
+    bool NeedsDwarfCFI = 
+         (MMI->hasDebugInfo() || MF->getFunction()->needsUnwindTableEntry()) && 
+         MMI->usePreciseUnwindInfo();
     int stackGrowth = -RI->getSlotSize();
 
     if (NeedsDwarfCFI && !hasFP) {
       OutStreamer->EmitCFIAdjustCfaOffset(-stackGrowth);
     }
 
     // Emit the label.
     OutStreamer->EmitLabel(PICBase);

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-pc-linux | FileCheck %s -check-prefix=LINUX -check-prefix=PRECISE -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 eeither 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*) {

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
 }

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