[PATCH] D17980: [lld] [ELF/AArch64] Add GD to IE TLS relax optimization

[Adhemerval Zanella via llvm-commits]

zatrazz added a comment.

I will update a new patch with fixes for your comments.


================
Comment at: ELF/Target.cpp:1538
@@ +1537,3 @@
+  //   ldr     x1, [x0, #:tlsdesc_lo12:v  [R_AARCH64_TLSDESC_LD64_LO12_NC]
+  //   add     x0, x0, :tlsdesc_los:v     [_AARCH64_TLSDESC_ADD_LO12_NC]
+  //   .tlsdesccall                       [R_AARCH64_TLSDESC_CALL]
----------------
grimar wrote:
> I think you missing ]
> 
> ```
> ldr     x1, [x0, #:tlsdesc_lo12:v ]
> ```
> 
> and "R_":
> 
> 
> ```
> R_AARCH64_TLSDESC_ADD_LO12_NC
> ```
Yeap, I will fix it.

================
Comment at: ELF/Target.cpp:1546
@@ +1545,3 @@
+
+  // For Initial-Exec from Global-Dynamic TLS relax the idea is to emit
+  // a R_AARCH64_TLS_TPREL64 dynamic relocation and thus adjust the
----------------
grimar wrote:
> Please remove the empty line.
Ok.

================
Comment at: ELF/Target.cpp:1550
@@ +1549,3 @@
+
+  checkUInt<32>(SA, Type);
+
----------------
grimar wrote:
> ditto.
Ok.

================
Comment at: ELF/Target.cpp:1562
@@ +1561,3 @@
+    updateAArch64Addr(Loc, (X >> 12) & 0x1FFFFF); // X[32:12]
+  } return;
+  case R_AARCH64_TLSDESC_LD64_LO12_NC: {
----------------
grimar wrote:
> could you put return inside {...} to be consistent with other lld code ?
> 
> 
> ```
>  case R_AARCH64_TLSDESC_ADR_PAGE21: {
>     // adrp
>     uint64_t X = getAArch64Page(SA) - getAArch64Page(P);
>     updateAArch64Addr(Loc, (X >> 12) & 0x1FFFFF); // X[32:12]
>     return;
>   } 
> ```
Ok.

================
Comment at: ELF/Target.cpp:1567-1569
@@ +1566,5 @@
+    NewInst |= (SA & 0xFF8) << 7;
+    write32le(Loc, NewInst);
+  } break;
+  default:
+    llvm_unreachable("Unsupported Relocation for TLS GD to LE relax");
----------------
grimar wrote:
> ditto for break
Ok.

================
Comment at: ELF/Target.cpp:1571
@@ +1570,3 @@
+    llvm_unreachable("Unsupported Relocation for TLS GD to LE relax");
+  }
+}
----------------
grimar wrote:
> not sure why "Relocation" is uppercase.
> I would write "Unsupported relocation for TLS GD to LE relaxation"
Right, and the wording is wrong, it should be "TLS GD to IE". I will change that.

================
Comment at: test/ELF/Inputs/aarch64-tls-ie.s:12
@@ -11,3 +10,2 @@
-.text
  .global bar
  .section .tdata,"awT",%progbits
----------------
grimar wrote:
> Why you do remove it in this patch ? its just excessive .text declaration it seems. I think it is not relative to the patch and should be fixed separatelly.
I just noted it is non required .text declaration and since it being used on the testcase being added to create the shared library I decided to cleanup this up. I can let as it and clean before or after this patch.

================
Comment at: test/ELF/aarch64-tls-gdie.s:7
@@ +6,3 @@
+# RUN: llvm-readobj -s -r %tout | FileCheck -check-prefix=RELOC %s
+# REQUIRES: aarch64
+
----------------
grimar wrote:
> Not an issue actually, but we often put the REQUIRES to be the first line of test. That looks better IMO
Right, I will change that.

================
Comment at: test/ELF/aarch64-tls-gdie.s:10
@@ +9,3 @@
+#Global-Dynamic to Initial-Exec relax creates a R_AARCH64_TPREL64
+# RELOC:      Relocations [
+# RELOC-NEXT:   Section ({{.*}}) .rela.dyn {
----------------
grimar wrote:
> Add a space here please "# Global-Dynamic..."
Ok.


Repository:
  rL LLVM

http://reviews.llvm.org/D17980