[PATCH] D68062: Propeller lld framework for basicblock sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Some suggestions for comments are inlined.



================
Comment at: lld/test/ELF/propeller/propeller-error-on-bblabels.s:103
+
+	.ident	"clang version 10.0.0 (git at github.com:google/llvm-propeller.git ff4b848f375a6a915f6fa0350f425c08a0212b19)"
+	.section	".note.GNU-stack","", at progbits
----------------
(A comment that applies to all test files in this patch.)

Scrub unnecessary assembly instructions and directives. Unnecessary directives include `.file`, `.ident`, `.addrsig`, etc. `.addrsig` is only useful if you want to test propeller's interaction with --icf=safe. `.file` is only necessary if the filename is significant.

Please also make basic blocks as simple as possible. The tests take more than 3000 lines of code. They will be very difficult to change if someone proposes a new feature that somehow interacts with propeller and has to fix all the tests.

For example, for the instructions below, one or two is probably sufficient. It is entirely unnecessary to have tens of instructions that are not relocated and are not used in any CHECK lines.

```
  movq  %rax, %rdx
  shrq  $63, %rdx
  sarq  $34, %rax
  addl  %edx, %eax
  addl  %eax, %eax
```

Some clang generated comments should also be removed, e.g.

`  movhpd  -8(%rsp), %xmm0         # xmm0 = xmm0[0],mem[0]`


When the test is long, besides the file-level comment `## `, add some inline comments. Again, most people are unfamiliar with the feature and the comment will help them if they have to adjust propeller tests.


================
Comment at: lld/test/ELF/propeller/propeller-keep-bb-symbols.s:1
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: ld.lld -propeller=%S/Inputs/propeller-2.data -propeller-dump-symbol-order=$(dirname %t.o)/symbol-order-2 %t.o -o %t.out
----------------
Most `-triple=x86_64-pc-linux` can be `-triple=x86_64`. They are applicable to all ELF platforms, not just Linux.


================
Comment at: lld/test/ELF/propeller/propeller-keep-bb-symbols.s:3
+# RUN: ld.lld -propeller=%S/Inputs/propeller-2.data -propeller-dump-symbol-order=$(dirname %t.o)/symbol-order-2 %t.o -o %t.out
+# RUN: cat $(dirname %t.o)/symbol-order-2 | FileCheck %s --check-prefix=SYM_ORDER
+# SYM_ORDER: Hot
----------------
I doubt `$(...)` (bash syntax) can be used in portable lit tests. There is no other `'RUN.*\$\('` usage. use %p (https://llvm.org/docs/CommandGuide/lit.html#pre-defined-substitutions)


================
Comment at: lld/test/ELF/propeller/propeller-keep-bb-symbols.s:8
+
+# Symbol "aaaaaa.BB.main" is removed because it is adjacent to aaaa.BB.main.
+# RUN: [[ -z `llvm-nm -S %t.out | grep -F "aaaaaa.BB.main"` ]]
----------------
Use `## ` for comments, `# RUN: ` for RUN lines or CHECK lines.


================
Comment at: lld/test/ELF/propeller/propeller-keep-bb-symbols.s:9
+# Symbol "aaaaaa.BB.main" is removed because it is adjacent to aaaa.BB.main.
+# RUN: [[ -z `llvm-nm -S %t.out | grep -F "aaaaaa.BB.main"` ]]
+# Symbol "aaaa.BB.main" is kept, because it starts a new cold code section for function "main".
----------------
`[[` is bash-ism. They will fail if the test runner does not use bash. Prefer FileCheck to grep.

When you don't need the size field, don't add -S to the llvm-nm command line.


================
Comment at: lld/test/ELF/propeller/propeller-opt-all-combinations.s:184
+# NO_SPLIT_FUNC-NEXT:	foo:
+# NO_SPLIT_FUNC-NEXT:	xorb	%al, 0
+# NO_SPLIT_FUNC-NEXT:	int3
----------------
Indent instructions as you do for instructions above.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68062/new/

https://reviews.llvm.org/D68062