[PowerPC] fix potential verification error on __tls_get_addr

[inouehrs]

Summary

This patch fixes a verification error with -verify-machineinstrs while expanding __tls_get_addr by not creating ADJCALLSTACKUP and ADJCALLSTACKDOWN if there is another ADJCALLSTACKUP in this basic block since nesting ADJCALLSTACKUP/ADJCALLSTACKDOWN is not allowed.

Here, ADJCALLSTACKUP and ADJCALLSTACKDOWN are created as a fence for instruction scheduling to avoid _tls_get_addr is scheduled before mflr in the prologue (https://bugs.llvm.org//show_bug.cgi?id=25839). So if another ADJCALLSTACKUP exists before _tls_get_addr, we do not need to create a new ADJCALLSTACKUP.

With this patch and two previous patches (https://reviews.llvm.org/D34208 and https://reviews.llvm.org/D34209), I successfully compiled LLVM+CLANG with -verify-machineinstrs on ppc64le Linux.

[nemanjai]

I think we should add a test case for this. Of course, since these are pseudo instructions we're dealing with, it would probably have to be an mir test.

lib/Target/PowerPC/PPCTLSDynamicCall.cpp

61 ↗

(On Diff #103019)

If the ADJCALLSTACKDOWN instruction is guaranteed to occur before any of the "TLS" pseudos in a basic block, that should be mentioned in a comment here. If on the other hand it isn't guaranteed to happen, we can form adjacent ADJCALLSTACKDOWN/ADJCALLSTACKUP blocks.

I don't imagine that's a problem, but it is somewhat inconsistent here...
What I mean is that if this loop encounters an ADJCALLSTACKDOWN which is followed by an ADJCALLSTACKUP, we will still not emit the instructions for the subsequent TLS instructions. I am not sure if that is a problem or not.

So the crux of what I'm saying here is this:

...
ADJCALLSTACKDOWN
...
ADJCALLSTACKUP
...
ADDItlsgdLADDR

Will not add the adjustments. Whereas this will (for all of the TLS instructions):

...
ADDItlsgdLADDR
...
ADDItlsgdLADDR

And this will add the adjustments only for the TLS instructions that appear before the stack adjustments that were already there:

...
ADDItlsgdLADDR
...
ADJCALLSTACKDOWN
...
ADJCALLSTACKUP
...
ADDItlsgdLADDR

[inouehrs]

• added MIR test cases
• added initialization of ppc-tls-dynamic-call pass to use with llc -run-pass
• updated the logic to check existing ADJCALLSTACKUP as well as ADJCALLSTACKDOWN

[inouehrs]

lib/Target/PowerPC/PPCTLSDynamicCall.cpp

61 ↗

(On Diff #103019)

I modified the logic to be more consistent. Now the following two cases both generate the adjustments.

...
ADJCALLSTACKDOWN
...
ADJCALLSTACKUP
...
ADDItlsgdLADDR

and

...
ADDItlsgdLADDR
...
ADDItlsgdLADDR

[nemanjai]

OK, this LGTM now. I'll approve this patch but give Tim and Kit a day or two to respond if they're interested in doing so before committing this.

[hfinkel]

lib/Target/PowerPC/PPCTLSDynamicCall.cpp

112 ↗

(On Diff #103238)

This comment does not explain the "Here, ADJCALLSTACKUP and ADJCALLSTACKDOWN are created as a fence for instruction scheduling to avoid _tls_get_addr is scheduled before mflr in the prologue". I realize that this is a pre-existing issue, but while you're here, we should document this better.

[inouehrs]

Touch up comments

[inouehrs]

lib/Target/PowerPC/PPCTLSDynamicCall.cpp

112 ↗

(On Diff #103238)

Thank you for the comment. I added some description on the intention of these ADJCALLSTACKDOWN/UP.

llvm/trunk/lib/Target/PowerPC/PPC.h

#endif
   FunctionPass *createPPCBoolRetToIntPass();
   FunctionPass *createPPCExpandISELPass();
   void LowerPPCMachineInstrToMCInst(const MachineInstr *MI, MCInst &OutMI,
                                     AsmPrinter &AP, bool isDarwin);
 
   void initializePPCVSXFMAMutatePass(PassRegistry&);
   void initializePPCBoolRetToIntPass(PassRegistry&);
   void initializePPCExpandISELPass(PassRegistry &);
+  void initializePPCTLSDynamicCallPass(PassRegistry &);
   extern char &PPCVSXFMAMutateID;
 
   namespace PPCII {
     
   /// Target Operand Flag enum.
   enum TOF {
     //===------------------------------------------------------------------===//
     // PPC Specific MachineOperand flags.

llvm/trunk/lib/Target/PowerPC/PPCTLSDynamicCall.cpp

struct PPCTLSDynamicCall : public MachineFunctionPass {
     }
 
     const PPCInstrInfo *TII;
     LiveIntervals *LIS;
 
 protected:
     bool processBlock(MachineBasicBlock &MBB) {
       bool Changed = false;
+      bool NeedFence = true;
       bool Is64Bit = MBB.getParent()->getSubtarget<PPCSubtarget>().isPPC64();
 
       for (MachineBasicBlock::iterator I = MBB.begin(), IE = MBB.end();
            I != IE;) {
         MachineInstr &MI = *I;
 
         if (MI.getOpcode() != PPC::ADDItlsgdLADDR &&
             MI.getOpcode() != PPC::ADDItlsldLADDR &&
             MI.getOpcode() != PPC::ADDItlsgdLADDR32 &&
             MI.getOpcode() != PPC::ADDItlsldLADDR32) {
+
+          // Although we create ADJCALLSTACKDOWN and ADJCALLSTACKUP
+          // as scheduling fences, we skip creating fences if we already
+          // have existing ADJCALLSTACKDOWN/UP to avoid nesting,
+          // which causes verification error with -verify-machineinstrs.
+          if (MI.getOpcode() == PPC::ADJCALLSTACKDOWN)
+            NeedFence = false;
+          else if (MI.getOpcode() == PPC::ADJCALLSTACKUP)
+            NeedFence = true;
+
           ++I;
           continue;
         }
 
         DEBUG(dbgs() << "TLS Dynamic Call Fixup:\n    " << MI);
 
         unsigned OutReg = MI.getOperand(0).getReg();
         unsigned InReg = MI.getOperand(1).getReg();
bool processBlock(MachineBasicBlock &MBB) {
           Opc2 = PPC::GETtlsADDR32;
           break;
         case PPC::ADDItlsldLADDR32:
           Opc1 = PPC::ADDItlsldL32;
           Opc2 = PPC::GETtlsldADDR32;
           break;
         }
 
-        // Don't really need to save data to the stack - the clobbered
+        // We create ADJCALLSTACKUP and ADJCALLSTACKDOWN around _tls_get_addr
+        // as schduling fence to avoid it is scheduled before
+        // mflr in the prologue and the address in LR is clobbered (PR25839).
+        // We don't really need to save data to the stack - the clobbered
         // registers are already saved when the SDNode (e.g. PPCaddiTlsgdLAddr)
         // gets translated to the pseudo instruction (e.g. ADDItlsgdLADDR).
+        if (NeedFence)
           BuildMI(MBB, I, DL, TII->get(PPC::ADJCALLSTACKDOWN)).addImm(0)
                                                               .addImm(0);
 
         // Expand into two ops built prior to the existing instruction.
         MachineInstr *Addi = BuildMI(MBB, I, DL, TII->get(Opc1), GPR3)
           .addReg(InReg);
         Addi->addOperand(MI.getOperand(2));
 
         // The ADDItls* instruction is the first instruction in the
         // repair range.
         MachineBasicBlock::iterator First = I;
         --First;
 
         MachineInstr *Call = (BuildMI(MBB, I, DL, TII->get(Opc2), GPR3)
                               .addReg(GPR3));
         Call->addOperand(MI.getOperand(3));
 
+        if (NeedFence)
           BuildMI(MBB, I, DL, TII->get(PPC::ADJCALLSTACKUP)).addImm(0).addImm(0);
 
         BuildMI(MBB, I, DL, TII->get(TargetOpcode::COPY), OutReg)
           .addReg(GPR3);
 
         // The COPY is the last instruction in the repair range.
         MachineBasicBlock::iterator Last = I;
         --Last;
 

llvm/trunk/lib/Target/PowerPC/PPCTargetMachine.cpp

extern "C" void LLVMInitializePowerPCTarget() {
   // Register the targets
   RegisterTargetMachine<PPCTargetMachine> A(getThePPC32Target());
   RegisterTargetMachine<PPCTargetMachine> B(getThePPC64Target());
   RegisterTargetMachine<PPCTargetMachine> C(getThePPC64LETarget());
 
   PassRegistry &PR = *PassRegistry::getPassRegistry();
   initializePPCBoolRetToIntPass(PR);
   initializePPCExpandISELPass(PR);
+  initializePPCTLSDynamicCallPass(PR);
 }
 
 /// Return the datalayout string of a subtarget.
 static std::string getDataLayoutString(const Triple &T) {
   bool is64Bit = T.getArch() == Triple::ppc64 || T.getArch() == Triple::ppc64le;
   std::string Ret;
 
   // Most PPC* platforms are big endian, PPC64LE is little endian.

llvm/trunk/test/CodeGen/PowerPC/tls_get_addr_fence1.mir

+# ADJCALLSTACKDOWN and ADJCALLSTACKUP must be generated around TLS pseudo code as scheduling fence (PR25839).
+# RUN: llc -mtriple=powerpc64le-linux-gnu -run-pass=ppc-tls-dynamic-call -verify-machineinstrs -o - %s | FileCheck %s
+
+--- |
+  target datalayout = "e-m:e-i64:64-n32:64"
+  target triple = "powerpc64le-unknown-linux-gnu"
+  
+  @tls_var = external thread_local local_unnamed_addr global i32
+  
+  define i32 @tls_func() local_unnamed_addr {
+  entry:
+    %0 = load i32, i32* @tls_var
+    ret i32 %0
+  }
+
+...
+---
+name:            tls_func
+alignment:       4
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+registers:       
+  - { id: 0, class: g8rc_and_g8rc_nox0, preferred-register: '' }
+  - { id: 1, class: g8rc_and_g8rc_nox0, preferred-register: '' }
+  - { id: 2, class: g8rc, preferred-register: '' }
+liveins:         
+  - { reg: '%x2' }
+frameInfo:       
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    false
+  hasCalls:        false
+  stackProtector:  ''
+  maxCallFrameSize: 4294967295
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:      
+stack:           
+constants:       
+body:             |
+  bb.0.entry:
+    liveins: %x2
+    %0 = ADDIStlsgdHA %x2, @tls_var
+    %1 = ADDItlsgdLADDR killed %0, @tls_var, @tls_var, implicit-def dead %x0, implicit-def dead %x3, implicit-def dead %x4, implicit-def dead %x5, implicit-def dead %x6, implicit-def dead %x7, implicit-def dead %x8, implicit-def dead %x9, implicit-def dead %x10, implicit-def dead %x11, implicit-def dead %x12, implicit-def dead %lr8, implicit-def dead %ctr8, implicit-def dead %cr0, implicit-def dead %cr1, implicit-def dead %cr5, implicit-def dead %cr6, implicit-def dead %cr7
+    %2 = LWZ8 0, killed %1 :: (dereferenceable load 4 from @tls_var)
+    %x3 = COPY %2
+    BLR8 implicit %lr8, implicit %rm, implicit %x3
+    ; CHECK-LABEL: bb.0.entry
+    ; CHECK: %[[reg1:[0-9]+]] = ADDIStlsgdHA %x2, @tls_var
+    ; CHECK: ADJCALLSTACKDOWN 0, 0
+    ; CHECK: %x3 = ADDItlsgdL %[[reg1]], @tls_var
+    ; CHECK: %x3 = GETtlsADDR %x3, @tls_var
+    ; CHECK: ADJCALLSTACKUP 0, 0
+    ; CHECK: BLR8
+...

llvm/trunk/test/CodeGen/PowerPC/tls_get_addr_fence2.mir

+# ADJCALLSTACKDOWN and ADJCALLSTACKUP should not be generated around TLS pseudo code if it is located within existing ADJCALLSTACKDOWN/ADJCALLSTACKUP pair.
+# RUN: llc -mtriple=powerpc64le-linux-gnu -run-pass=ppc-tls-dynamic-call -verify-machineinstrs -o - %s | FileCheck %s
+
+--- |
+  target datalayout = "e-m:e-i64:64-n32:64"
+  target triple = "powerpc64le-unknown-linux-gnu"
+  
+  @tls_var = external thread_local local_unnamed_addr global i32
+  
+  define i32 @tls_func() local_unnamed_addr {
+  entry:
+    %0 = load i32, i32* @tls_var
+    ret i32 %0
+  }
+
+...
+---
+name:            tls_func
+alignment:       4
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+registers:       
+  - { id: 0, class: g8rc_and_g8rc_nox0, preferred-register: '' }
+  - { id: 1, class: g8rc_and_g8rc_nox0, preferred-register: '' }
+  - { id: 2, class: g8rc, preferred-register: '' }
+liveins:         
+  - { reg: '%x2' }
+frameInfo:       
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    false
+  hasCalls:        false
+  stackProtector:  ''
+  maxCallFrameSize: 4294967295
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:      
+stack:           
+constants:       
+body:             |
+  bb.0.entry:
+    liveins: %x2
+    ADJCALLSTACKDOWN 32, 0, implicit-def %r1, implicit %r1
+    %0 = ADDIStlsgdHA %x2, @tls_var
+    %1 = ADDItlsgdLADDR killed %0, @tls_var, @tls_var, implicit-def dead %x0, implicit-def dead %x3, implicit-def dead %x4, implicit-def dead %x5, implicit-def dead %x6, implicit-def dead %x7, implicit-def dead %x8, implicit-def dead %x9, implicit-def dead %x10, implicit-def dead %x11, implicit-def dead %x12, implicit-def dead %lr8, implicit-def dead %ctr8, implicit-def dead %cr0, implicit-def dead %cr1, implicit-def dead %cr5, implicit-def dead %cr6, implicit-def dead %cr7
+    %2 = LWZ8 0, killed %1 :: (dereferenceable load 4 from @tls_var)
+    %x3 = COPY %2
+    ADJCALLSTACKUP 32, 0, implicit-def %r1, implicit %r1
+    BLR8 implicit %lr8, implicit %rm, implicit %x3
+    ; CHECK-LABEL: bb.0.entry
+    ; CHECK-NOT: ADJCALLSTACKDOWN 0, 0
+    ; CHECK-NOT: ADJCALLSTACKUP 0, 0
+    ; CHECK: BLR8
+...