[X86ISelLowering] Fix TLSADDR lowering when shrink-wrapping is enabled

[davide]

Summary

This is one of the blockers for the release. Testcase will follow. I'm running 'check-llvm' now.

[qcolombet]

Hi Davide,

Thanks for fixing this.
To give a bit of context here for other reviewers, the bug is that TLSADDR are lowered into calls in MC. As a result, we need to put the “call markers” around those instructions, otherwise shrink-wrapping may push the prologue and epilogue pass them.

This mostly looks good to me see my inline comment. It misses a test case of course, but I know you’re working on it.

Cheers,
-Quentin

lib/Target/X86/X86ISelLowering.cpp

22665

Add a comment saying that we want to replace TLSADDR into
adjust_stackdown
TLSADDR
adjust_stackup
and why. (You may use what I said to explain the problem.)

22675

"after the instruction", instead of “at the end of the basic block”.

[davide]

• Addressed Quentin's comments.
• Added a test.

This passes llvm-check.

[davide]

• Fix test.

[qcolombet]

Hi Davide,

LGTM with a few nitpicks on the test case.

Thanks for fixing it!
Cheers,
-Quentin

lib/Target/X86/X86ISelLowering.cpp

22680

two spaces after CALLSEQ_END.

test/CodeGen/X86/tls-shrink-wrapping.ll

10

Add a comment on what this is testing.
In case the test breaks in the future, eg because of code gen changes, we want to know what was the intended behavior.

12

Please use opt -instnamer on the test case to have temporary variable names (easier to update than %[0-9]+).

12

Please get rid of the metadata (!…) unless they are required to reproduce the problem.

56

You should be able to remove all the !… stuff after removing the !… from the function.

lib/Target/X86/X86ISelLowering.h

                                            MachineBasicBlock *BB) const;
 
     MachineBasicBlock *EmitLoweredCatchPad(MachineInstr *MI,
                                            MachineBasicBlock *BB) const;
 
     MachineBasicBlock *EmitLoweredSegAlloca(MachineInstr *MI,
                                             MachineBasicBlock *BB) const;
 
+    MachineBasicBlock *EmitLoweredTLSAddr(MachineInstr *MI,
+                                          MachineBasicBlock *BB) const;
+
     MachineBasicBlock *EmitLoweredTLSCall(MachineInstr *MI,
                                           MachineBasicBlock *BB) const;
 
     MachineBasicBlock *emitEHSjLjSetJmp(MachineInstr *MI,
                                         MachineBasicBlock *MBB) const;
 
     MachineBasicBlock *emitEHSjLjLongJmp(MachineInstr *MI,
                                          MachineBasicBlock *MBB) const;

lib/Target/X86/X86ISelLowering.cpp

     DebugLoc DL = MI->getDebugLoc();
     BuildMI(*BB, MI, DL, TII.get(X86::EH_RESTORE));
   }
   MI->eraseFromParent();
   return BB;
 }
 
 MachineBasicBlock *
+X86TargetLowering::EmitLoweredTLSAddr(MachineInstr *MI,
+                                      MachineBasicBlock *BB) const {
+  // So, here we replace TLSADDR with the sequence:
+  // adjust_stackdown -> TLSADDR -> adjust_stackup.
+  // We need this because TLSADDR is lowered into calls
+  // inside MC, therefore without the two markers shrink-wrapping
+  // may push the prologue/epilogue pass them.
+  const TargetInstrInfo &TII = *Subtarget.getInstrInfo();
+  DebugLoc DL = MI->getDebugLoc();
+  MachineFunction &MF = *BB->getParent();
+
+  // Emit CALLSEQ_START right before the instruction.
+  unsigned AdjStackDown = TII.getCallFrameSetupOpcode();
+  MachineInstrBuilder CallseqStart =
+    BuildMI(MF, DL, TII.get(AdjStackDown)).addImm(0);
+  BB->insert(MachineBasicBlock::iterator(MI), CallseqStart);
+
+  // Emit CALLSEQ_END  right after the instruction.
+  // We don't call erase from parent because we want to keep the
+  // original instruction around.
+  unsigned AdjStackUp = TII.getCallFrameDestroyOpcode();
+  MachineInstrBuilder CallseqEnd =
+    BuildMI(MF, DL, TII.get(AdjStackUp)).addImm(0).addImm(0);
+  BB->insertAfter(MachineBasicBlock::iterator(MI), CallseqEnd);
+
+  return BB;
+}
+
+MachineBasicBlock *
 X86TargetLowering::EmitLoweredTLSCall(MachineInstr *MI,
                                       MachineBasicBlock *BB) const {
   // This is pretty easy.  We're taking the value that we received from
   // our load from the relocation, sticking it in either RDI (x86-64)
   // or EAX and doing an indirect call.  The return value will then
   // be in the normal return register.
   MachineFunction *F = BB->getParent();
   const X86InstrInfo *TII = Subtarget.getInstrInfo();

   case X86::TAILJMPd64_REX:
   case X86::TAILJMPr64_REX:
   case X86::TAILJMPm64_REX:
     llvm_unreachable("TAILJMP64 would not be touched here.");
   case X86::TCRETURNdi64:
   case X86::TCRETURNri64:
   case X86::TCRETURNmi64:
     return BB;
+  case X86::TLS_addr32:
+  case X86::TLS_addr64:
+  case X86::TLS_base_addr32:
+  case X86::TLS_base_addr64:
+    return EmitLoweredTLSAddr(MI, BB);
   case X86::WIN_ALLOCA:
     return EmitLoweredWinAlloca(MI, BB);
   case X86::CATCHRET:
     return EmitLoweredCatchRet(MI, BB);
   case X86::CATCHPAD:
     return EmitLoweredCatchPad(MI, BB);
   case X86::SEG_ALLOCA_32:
   case X86::SEG_ALLOCA_64:

lib/Target/X86/X86InstrCompiler.td

 // All calls clobber the non-callee saved registers. ESP is marked as
 // a use to prevent stack-pointer assignments that appear immediately
 // before calls from potentially appearing dead.
 let Defs = [EAX, ECX, EDX, FP0, FP1, FP2, FP3, FP4, FP5, FP6, FP7,
             ST0, ST1, ST2, ST3, ST4, ST5, ST6, ST7,
             MM0, MM1, MM2, MM3, MM4, MM5, MM6, MM7,
             XMM0, XMM1, XMM2, XMM3, XMM4, XMM5, XMM6, XMM7,
             XMM8, XMM9, XMM10, XMM11, XMM12, XMM13, XMM14, XMM15, EFLAGS],
-    Uses = [ESP] in {
+    usesCustomInserter = 1, Uses = [ESP] in {
 def TLS_addr32 : I<0, Pseudo, (outs), (ins i32mem:$sym),
                   "# TLS_addr32",
                   [(X86tlsaddr tls32addr:$sym)]>,
                   Requires<[Not64BitMode]>;
 def TLS_base_addr32 : I<0, Pseudo, (outs), (ins i32mem:$sym),
                   "# TLS_base_addr32",
                   [(X86tlsbaseaddr tls32baseaddr:$sym)]>,
                   Requires<[Not64BitMode]>;
 }
 
 // All calls clobber the non-callee saved registers. RSP is marked as
 // a use to prevent stack-pointer assignments that appear immediately
 // before calls from potentially appearing dead.
 let Defs = [RAX, RCX, RDX, RSI, RDI, R8, R9, R10, R11,
             FP0, FP1, FP2, FP3, FP4, FP5, FP6, FP7,
             ST0, ST1, ST2, ST3, ST4, ST5, ST6, ST7,
             MM0, MM1, MM2, MM3, MM4, MM5, MM6, MM7,
             XMM0, XMM1, XMM2, XMM3, XMM4, XMM5, XMM6, XMM7,
             XMM8, XMM9, XMM10, XMM11, XMM12, XMM13, XMM14, XMM15, EFLAGS],
-    Uses = [RSP] in {
+    usesCustomInserter = 1, Uses = [RSP] in {
 def TLS_addr64 : I<0, Pseudo, (outs), (ins i64mem:$sym),
                    "# TLS_addr64",
                   [(X86tlsaddr tls64addr:$sym)]>,
                   Requires<[In64BitMode]>;
 def TLS_base_addr64 : I<0, Pseudo, (outs), (ins i64mem:$sym),
                    "# TLS_base_addr64",
                   [(X86tlsbaseaddr tls64baseaddr:$sym)]>,
                   Requires<[In64BitMode]>;

test/CodeGen/X86/tls-shrink-wrapping.ll

+; RUN: llc < %s -march=x86-64 -mtriple=x86_64-linux-gnu -relocation-model=pic | FileCheck %s
+
+@i = external thread_local global i32, align 4
+
+define i32 @g() {
+entry:
+  %0 = load i32, i32* @i, align 4, !tbaa !2
+  %tobool = icmp eq i32 %0, 0
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:                                          ; preds = %entry
+  store i32 0, i32* @i, align 4, !tbaa !2
+  tail call void (...) @f()
+  %.pre = load i32, i32* @i, align 4, !tbaa !2
+  br label %if.end
+
+if.end:                                           ; preds = %entry, %if.then
+  %1 = phi i32 [ 0, %entry ], [ %.pre, %if.then ]
+  ret i32 %1
+}
+
+; CHECK: g:                                      # @g
+; CHECK-NEXT:         .cfi_startproc
+; CHECK-NEXT: # BB#0:                                 # %entry
+; CHECK-NEXT:         pushq   %rbx
+; CHECK-NEXT: .Ltmp0:
+; CHECK-NEXT:         .cfi_def_cfa_offset 16
+; CHECK-NEXT: .Ltmp1:
+; CHECK-NEXT:         .cfi_offset %rbx, -16
+; CHECK-NEXT:         data16
+; CHECK-NEXT:         leaq    i@TLSGD(%rip), %rdi
+; CHECK-NEXT:         data16
+; CHECK-NEXT:         data16
+; CHECK-NEXT:         rex64
+; CHECK-NEXT:         callq   __tls_get_addr@PLT
+
+declare void @f(...)
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"PIC Level", i32 2}
+!1 = !{!"clang version 3.9.0 (trunk 261236)"}
+!2 = !{!3, !3, i64 0}
+!3 = !{!"int", !4, i64 0}
+!4 = !{!"omnipotent char", !5, i64 0}
+!5 = !{!"Simple C/C++ TBAA"}