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

[George Rimar via llvm-commits]

grimar added a comment.

IMO that is cool if this helps to "bootstrap and run all the lld tests without failure" !


================
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]
----------------
I think you missing ]

```
ldr     x1, [x0, #:tlsdesc_lo12:v ]
```

and "R_":


```
R_AARCH64_TLSDESC_ADD_LO12_NC
```

================
Comment at: ELF/Target.cpp:1539
@@ +1538,3 @@
+  //   add     x0, x0, :tlsdesc_los:v     [_AARCH64_TLSDESC_ADD_LO12_NC]
+  //   .tlsdesccall                       [R_AARCH64_TLSDESC_CALL]
+  // And it can optimized to:
----------------
```
.tlsdesccall    v
```

================
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
----------------
Please remove the empty line.

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

================
Comment at: ELF/Target.cpp:1562
@@ +1561,3 @@
+    updateAArch64Addr(Loc, (X >> 12) & 0x1FFFFF); // X[32:12]
+  } return;
+  case R_AARCH64_TLSDESC_LD64_LO12_NC: {
----------------
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;
  } 
```

================
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");
----------------
ditto for break

================
Comment at: ELF/Target.cpp:1571
@@ +1570,3 @@
+    llvm_unreachable("Unsupported Relocation for TLS GD to LE relax");
+  }
+}
----------------
not sure why "Relocation" is uppercase.
I would write "Unsupported relocation for TLS GD to LE relaxation"

================
Comment at: test/ELF/Inputs/aarch64-tls-ie.s:12
@@ -11,3 +10,2 @@
-.text
  .global bar
  .section .tdata,"awT",%progbits
----------------
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.

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

================
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 {
----------------
Add a space here please "# Global-Dynamic..."


Repository:
  rL LLVM

http://reviews.llvm.org/D17980