[AArch64] Fold adds with tprel_lo12_nc and secrel_lo12 into a following ldr/str

[rengolin]

Can't this be done in the DAGCombine?

[mstorsjo]

I don't know - this is the way it's done currently for other, normal global variable: An ADDlow can get merged into a following LDR/STR. For cases when the ADDlow isn't merged or matched by something else, there needs to be a fallback rule that lowers it.

I guess I could look at adding other code for matching an ADDXri machine node with LDR/STR, but I don't know if that has got other implications.

[sebpop]

I think this change is good.

Here is what happens on arm64-tls-execs.ll
before this patch, the dag after instruction selection looks like this:

t0: ch = EntryToken
      t12: i64 = ADDXri MOVbaseTLS:i64, TargetGlobalTLSAddress:i64<i32* @local_exec_var> 0 [TF=71], TargetConstant:i32<0>
    t13: i64 = ADDXri t12, TargetGlobalTLSAddress:i64<i32* @local_exec_var> 0 [TF=98], TargetConstant:i32<0>
  t4: i32,ch = LDRWui<Mem:LD4[@local_exec_var](dereferenceable)> t13, TargetConstant:i64<0>, t0
t6: ch,glue = CopyToReg t0, Register:i32 $w0, t4
t7: ch = RET_ReallyLR Register:i32 $w0, t6, t6:1

with the patch there is one less ADDXri that got folded into the load:

t0: ch = EntryToken
    t12: i64 = ADDXri MOVbaseTLS:i64, TargetGlobalTLSAddress:i64<i32* @local_exec_var> 0 [TF=71], TargetConstant:i32<0>
  t4: i32,ch = LDRWui<Mem:LD4[@local_exec_var](dereferenceable)> t12, TargetGlobalTLSAddress:i64<i32* @local_exec_var> 0 [TF=98], t0
t6: ch,glue = CopyToReg t0, Register:i32 $w0, t4
t7: ch = RET_ReallyLR Register:i32 $w0, t6, t6:1

That is because aarch64 has a pattern

defm : ExtLoadTo32ROPat<ro8,  extloadi8,   LDRBBroW, LDRBBroX>;

to match

  t13: i64 = AArch64ISD::ADDlow t12, TargetGlobalTLSAddress:i64<i32* @local_exec_var> 0 [TF=98]
t4: i32,ch = load<LD4[@local_exec_var](dereferenceable)> t0, t13, undef:i64

and transform that into:

Morphed node: t4: i32,ch = LDRWui<Mem:LD4[@local_exec_var](dereferenceable)> t12, TargetGlobalTLSAddress:i64<i32* @local_exec_var> 0 [TF=98], t0

I guess I could look at adding other code for matching an ADDXri machine node with LDR/STR, but I don't know if that has got other implications.

I think it is impossible to specify a pattern to match a load with a machine node AArch64::ADDXri.
The LHS matching part of a def-pat should be a generic dag node.

The current patch avoids lowering the add into a machine node too early, and keeps the add as a generic addlow node, making the load+addlow ISEL pattern match.
If the addlow node is not folded into a load, it gets caught by the pseudo after regalloc and lowered into a machine node ADDXri.

[peter.smith]

Unfortunately this change looks like it is going to be incompatible with binutils and LLD for the ELF targets as the ldr . The new output via llvm-objdump is:

       0:	48 d0 3b d5 	mrs	x8, TPIDR_EL0
       4:	08 01 40 91 	add	x8, x8, #0, lsl #12
		0000000000000004:  R_AARCH64_TLSLE_ADD_TPREL_HI12	tlsVar
       8:	00 01 40 b9 	ldr	w0, [x8]
		0000000000000008:  R_AARCH64_TLSLE_LDST32_TPREL_LO12_NC	tlsVar
       c:	c0 03 5f d6 	ret

The GNU tools and LLD don't implement R_AARCH64_TLSLE_LDST32_TPREL_LO12_NC yet. For example using aarch64-linux-gnu-objdump:

0000000000000000 <getVar>:
   0:	d53bd048 	mrs	x8, tpidr_el0
   4:	91400108 	add	x8, x8, #0x0, lsl #12
			4: R_AARCH64_TLSLE_ADD_TPREL_HI12	tlsVar
   8:	b9400100 	ldr	w0, [x8]
			8: *unknown*	tlsVar
   c:	d65f03c0 	ret

Whereas the equivalent add relocation R_AARCH64_TLSLE_ADD_TPREL_LO12 is supported.

This is currently manifesting itself as a buildbot failure on AArch64 http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/4609/steps/ninja%20check%202/logs/stdio

I think we'll need to get R_AARCH64_TLSLE_ADD_TPREL_LO12 implemented in binutils and LLD and widely adopted by distributions before we can enable this by default, at least for Linux targets.

[mstorsjo]

In D44355#1036858, @peter.smith wrote:

Unfortunately this change looks like it is going to be incompatible with binutils and LLD for the ELF targets as the ldr . The new output via llvm-objdump is:

       0:	48 d0 3b d5 	mrs	x8, TPIDR_EL0
       4:	08 01 40 91 	add	x8, x8, #0, lsl #12
		0000000000000004:  R_AARCH64_TLSLE_ADD_TPREL_HI12	tlsVar
       8:	00 01 40 b9 	ldr	w0, [x8]
		0000000000000008:  R_AARCH64_TLSLE_LDST32_TPREL_LO12_NC	tlsVar
       c:	c0 03 5f d6 	ret

The GNU tools and LLD don't implement R_AARCH64_TLSLE_LDST32_TPREL_LO12_NC yet. For example using aarch64-linux-gnu-objdump:

0000000000000000 <getVar>:
   0:	d53bd048 	mrs	x8, tpidr_el0
   4:	91400108 	add	x8, x8, #0x0, lsl #12
			4: R_AARCH64_TLSLE_ADD_TPREL_HI12	tlsVar
   8:	b9400100 	ldr	w0, [x8]
			8: *unknown*	tlsVar
   c:	d65f03c0 	ret

Whereas the equivalent add relocation R_AARCH64_TLSLE_ADD_TPREL_LO12 is supported.

This is currently manifesting itself as a buildbot failure on AArch64 http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/4609/steps/ninja%20check%202/logs/stdio

I think we'll need to get R_AARCH64_TLSLE_ADD_TPREL_LO12 implemented in binutils and LLD and widely adopted by distributions before we can enable this by default, at least for Linux targets.

Oh, I hadn't realized that this relocation wasn't implemented for ELF even though it did exist in the LLVM object library. When working on TLS for windows, I implemented both the ADD and LDR/STR relocations at the same time, and MSVC uses the LDR/STR form by default.

I'm fine with reverting the ELF part of this change for now - I'll do that in a moment.

[mstorsjo]

In D44355#1036871, @mstorsjo wrote:

In D44355#1036858, @peter.smith wrote:

I think we'll need to get R_AARCH64_TLSLE_ADD_TPREL_LO12 implemented in binutils and LLD and widely adopted by distributions before we can enable this by default, at least for Linux targets.

I'm fine with reverting the ELF part of this change for now - I'll do that in a moment.

Reverted in SVN r327503.

[peter.smith]

Thanks.

I've raised https://bugs.llvm.org/show_bug.cgi?id=36727 to get the relocation implemented in LLD. Will need to raise issues in Gold and BFD as well.

[peter.smith]

https://sourceware.org/bugzilla/show_bug.cgi?id=22969 and https://sourceware.org/bugzilla/show_bug.cgi?id=22970 raised on gold and ld.bfd respectively.

llvm/trunk/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

MachineInstrBuilder MIB2 =
             .addReg(DstReg)
             .add(MI.getOperand(2))
             .addImm(0);
 
     transferImpOps(MI, MIB1, MIB2);
     MI.eraseFromParent();
     return true;
   }
+  case AArch64::ADDlowTLS:
+    // Produce a plain ADD
+    BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(AArch64::ADDXri))
+        .add(MI.getOperand(0))
+        .add(MI.getOperand(1))
+        .add(MI.getOperand(2))
+        .addImm(0);
+    MI.eraseFromParent();
+    return true;
+
   case AArch64::MOVbaseTLS: {
     unsigned DstReg = MI.getOperand(0).getReg();
     auto SysReg = AArch64SysReg::TPIDR_EL0;
     MachineFunction *MF = MBB.getParent();
     if (MF->getTarget().getTargetTriple().isOSFuchsia() &&
         MF->getTarget().getCodeModel() == CodeModel::Kernel)
       SysReg = AArch64SysReg::TPIDR_EL1;
     BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(AArch64::MRS), DstReg)

llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp

SDValue LoVar = DAG.getTargetGlobalAddress(
         AArch64II::MO_TLS | AArch64II::MO_PAGEOFF | AArch64II::MO_NC);
 
     SDValue TPWithOff_lo =
         SDValue(DAG.getMachineNode(AArch64::ADDXri, DL, PtrVT, ThreadBase,
                                    HiVar,
                                    DAG.getTargetConstant(0, DL, MVT::i32)),
                 0);
     SDValue TPWithOff =
-        SDValue(DAG.getMachineNode(AArch64::ADDXri, DL, PtrVT, TPWithOff_lo,
-                                   LoVar,
-                                   DAG.getTargetConstant(0, DL, MVT::i32)),
-                0);
+        DAG.getNode(AArch64ISD::ADDlow, DL, PtrVT, TPWithOff_lo, LoVar);
     return TPWithOff;
   } else if (Model == TLSModel::InitialExec) {
     TPOff = DAG.getTargetGlobalAddress(GV, DL, PtrVT, 0, AArch64II::MO_TLS);
     TPOff = DAG.getNode(AArch64ISD::LOADgot, DL, PtrVT, TPOff);
   } else if (Model == TLSModel::LocalDynamic) {
     // Local-dynamic accesses proceed in two phases. A general-dynamic TLS
     // descriptor call against the special symbol _TLS_MODULE_BASE_ to calculate
     // the beginning of the module's TLS region, followed by a DTPREL offset
SDValue HiVar = DAG.getTargetGlobalAddress(
         GV, DL, MVT::i64, 0, AArch64II::MO_TLS | AArch64II::MO_HI12);
     SDValue LoVar = DAG.getTargetGlobalAddress(
         GV, DL, MVT::i64, 0,
         AArch64II::MO_TLS | AArch64II::MO_PAGEOFF | AArch64II::MO_NC);
 
     TPOff = SDValue(DAG.getMachineNode(AArch64::ADDXri, DL, PtrVT, TPOff, HiVar,
                                        DAG.getTargetConstant(0, DL, MVT::i32)),
                     0);
-    TPOff = SDValue(DAG.getMachineNode(AArch64::ADDXri, DL, PtrVT, TPOff, LoVar,
-                                       DAG.getTargetConstant(0, DL, MVT::i32)),
-                    0);
+    TPOff = DAG.getNode(AArch64ISD::ADDlow, DL, PtrVT, TPOff, LoVar);
   } else if (Model == TLSModel::GeneralDynamic) {
     // The call needs a relocation too for linker relaxation. It doesn't make
     // sense to call it MO_PAGE or MO_PAGEOFF though so we need another copy of
     // the address.
     SDValue SymAddr =
         DAG.getTargetGlobalAddress(GV, DL, PtrVT, 0, AArch64II::MO_TLS);
 
     // Finally we can make a call to calculate the offset from tpidr_el0.
SDValue TGALo = DAG.getTargetGlobalAddress(
       GV, DL, PtrVT, 0,
       AArch64II::MO_TLS | AArch64II::MO_PAGEOFF | AArch64II::MO_NC);
 
   // Add the offset from the start of the .tls section (section base).
   SDValue Addr =
       SDValue(DAG.getMachineNode(AArch64::ADDXri, DL, PtrVT, TLS, TGAHi,
                                  DAG.getTargetConstant(0, DL, MVT::i32)),
               0);
-  Addr = SDValue(DAG.getMachineNode(AArch64::ADDXri, DL, PtrVT, Addr, TGALo,
-                                    DAG.getTargetConstant(0, DL, MVT::i32)),
-                 0);
+  Addr = DAG.getNode(AArch64ISD::ADDlow, DL, PtrVT, Addr, TGALo);
   return Addr;
 }
 
 SDValue AArch64TargetLowering::LowerGlobalTLSAddress(SDValue Op,
                                                      SelectionDAG &DAG) const {
   const GlobalAddressSDNode *GA = cast<GlobalAddressSDNode>(Op);
   if (DAG.getTarget().useEmulatedTLS())
     return LowerToTLSEmulatedModel(GA, DAG);

llvm/trunk/lib/Target/AArch64/AArch64InstrInfo.td

: Pseudo<(outs GPR64:$dst), (ins i64imm:$hi, i64imm:$low),
              [(set GPR64:$dst, (AArch64addlow (AArch64adrp tglobaltlsaddr:$hi),
                                             tglobaltlsaddr:$low))]>,
       Sched<[WriteAdrAdr]>;
 def MOVaddrEXT
     : Pseudo<(outs GPR64:$dst), (ins i64imm:$hi, i64imm:$low),
              [(set GPR64:$dst, (AArch64addlow (AArch64adrp texternalsym:$hi),
                                             texternalsym:$low))]>,
       Sched<[WriteAdrAdr]>;
+// Normally AArch64addlow either gets folded into a following ldr/str,
+// or together with an adrp into MOVaddr above. For cases with TLS, it
+// might appear without either of them, so allow lowering it into a plain
+// add.
+def ADDlowTLS
+    : Pseudo<(outs GPR64:$dst), (ins GPR64:$src, i64imm:$low),
+             [(set GPR64:$dst, (AArch64addlow GPR64:$src,
+                                            tglobaltlsaddr:$low))]>,
+      Sched<[WriteAdr]>;
 
 } // isReMaterializable, isCodeGenOnly
 
 def : Pat<(AArch64LOADgot tglobaltlsaddr:$addr),
           (LOADgot tglobaltlsaddr:$addr)>;
 
 def : Pat<(AArch64LOADgot texternalsym:$addr),
           (LOADgot texternalsym:$addr)>;

llvm/trunk/test/CodeGen/AArch64/arm64-tls-execs.ll

 @local_exec_var = thread_local(localexec) global i32 0
 
 define i32 @test_local_exec() {
 ; CHECK-LABEL: test_local_exec:
   %val = load i32, i32* @local_exec_var
 
 ; CHECK: mrs x[[R1:[0-9]+]], TPIDR_EL0
 ; CHECK: add x[[R2:[0-9]+]], x[[R1]], :tprel_hi12:local_exec_var
-; CHECK: add x[[R3:[0-9]+]], x[[R2]], :tprel_lo12_nc:local_exec_var
-; CHECK: ldr w0, [x[[R3]]]
+; CHECK: ldr w0, [x[[R2]], :tprel_lo12_nc:local_exec_var]
 
 ; CHECK-RELOC: R_AARCH64_TLSLE_ADD_TPREL_HI12
-; CHECK-RELOC: R_AARCH64_TLSLE_ADD_TPREL_LO12_NC
+; CHECK-RELOC: R_AARCH64_TLSLE_LDST32_TPREL_LO12_NC
   ret i32 %val
 }
 
 define i32* @test_local_exec_addr() {
 ; CHECK-LABEL: test_local_exec_addr:
   ret i32* @local_exec_var
 
 ; CHECK: mrs x[[R1:[0-9]+]], TPIDR_EL0
 ; CHECK: add x[[R2:[0-9]+]], x[[R1]], :tprel_hi12:local_exec_var
 ; CHECK: add x0, x[[R2]], :tprel_lo12_nc:local_exec_var
 ; CHECK: ret
 
 ; CHECK-RELOC: R_AARCH64_TLSLE_ADD_TPREL_HI12
 ; CHECK-RELOC: R_AARCH64_TLSLE_ADD_TPREL_LO12_NC
 }

llvm/trunk/test/CodeGen/AArch64/win-tls.ll

 
 ; CHECK-LABEL: getVar
 ; CHECK: adrp [[TLS_INDEX_ADDR:x[0-9]+]], _tls_index
 ; CHECK: ldr w[[TLS_INDEX:[0-9]+]], {{\[}}[[TLS_INDEX_ADDR]], _tls_index]
 ; CHECK: ldr [[TLS_POINTER:x[0-9]+]], [x18, #88]
 
 ; CHECK: ldr [[TLS:x[0-9]+]], {{\[}}[[TLS_POINTER]], x[[TLS_INDEX]], lsl #3]
 ; CHECK: add [[TLS]], [[TLS]], :secrel_hi12:tlsVar
-; This add+ldr could also be folded into a single ldr with a :secrel_lo12:
-; offset.
-; CHECK: add [[TLS]], [[TLS]], :secrel_lo12:tlsVar
-; CHECK: ldr w0, {{\[}}[[TLS]]{{\]}}
+; CHECK: ldr w0, {{\[}}[[TLS]], :secrel_lo12:tlsVar{{\]}}
 
 ; CHECK-LABEL: getPtr
 ; CHECK: adrp [[TLS_INDEX_ADDR:x[0-9]+]], _tls_index
 ; CHECK: ldr w[[TLS_INDEX:[0-9]+]], {{\[}}[[TLS_INDEX_ADDR]], _tls_index]
 ; CHECK: ldr [[TLS_POINTER:x[0-9]+]], [x18, #88]
 
 ; CHECK: ldr [[TLS:x[0-9]+]], {{\[}}[[TLS_POINTER]], x[[TLS_INDEX]], lsl #3]
 ; CHECK: add [[TLS]], [[TLS]], :secrel_hi12:tlsVar
 ; CHECK: add x0, [[TLS]], :secrel_lo12:tlsVar
 
 ; CHECK-LABEL: setVar
 ; CHECK: adrp [[TLS_INDEX_ADDR:x[0-9]+]], _tls_index
 ; CHECK: ldr w[[TLS_INDEX:[0-9]+]], {{\[}}[[TLS_INDEX_ADDR]], _tls_index]
 ; CHECK: ldr [[TLS_POINTER:x[0-9]+]], [x18, #88]
 
 ; CHECK: ldr [[TLS:x[0-9]+]], {{\[}}[[TLS_POINTER]], x[[TLS_INDEX]], lsl #3]
 ; CHECK: add [[TLS]], [[TLS]], :secrel_hi12:tlsVar
-; This add+ldr could also be folded into a single ldr with a :secrel_lo12:
-; offset.
-; CHECK: add [[TLS]], [[TLS]], :secrel_lo12:tlsVar
-; CHECK: str w0, {{\[}}[[TLS]]{{\]}}
+; CHECK: str w0, {{\[}}[[TLS]], :secrel_lo12:tlsVar{{\]}}
 
 ; CHECK-LABEL: getVar8
 ; CHECK: add [[TLS:x[0-9]+]], [[TLS]], :secrel_hi12:tlsVar8
-; CHECK: add [[TLS]], [[TLS]], :secrel_lo12:tlsVar8
-; CHECK: ldrb w0, {{\[}}[[TLS]]{{\]}}
+; CHECK: ldrb w0, {{\[}}[[TLS]], :secrel_lo12:tlsVar8{{\]}}
 
 ; CHECK-LABEL: getVar64
 ; CHECK: add [[TLS:x[0-9]+]], [[TLS]], :secrel_hi12:tlsVar64
-; CHECK: add [[TLS]], [[TLS]], :secrel_lo12:tlsVar64
-; CHECK: ldr x0, {{\[}}[[TLS]]{{\]}}
+; CHECK: ldr x0, {{\[}}[[TLS]], :secrel_lo12:tlsVar64{{\]}}