[PATCH] D70157: Align branches within 32-Byte boundary

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:173
+
+  bool needAlignBranch() const;
+  bool needAlignJcc() const;
----------------
We can generalize these functions with a function that takes a parameter.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:411
+          cast<MCSymbolRefExpr>(*Operand.getExpr()).getSymbol().getName();
+      if (SymbolName.contains("__tls_get_addr"))
+        return true;
----------------
Check 64bit, then use exact comparison with either `___tls_get_addr` or `__tls_get_addr`


================
Comment at: llvm/test/MC/X86/i386-align-branch-1a.s:1
+# RUN: llvm-mc -filetype=obj -triple i386-unknown-unknown --x86-align-branch-boundary=32 --x86-align-branch=fused-jcc-jmp  --x86-align-branch-prefix-size=5 %s | llvm-objdump -d  - | FileCheck %s
+# RUN: llvm-mc -filetype=obj -triple i386-unknown-unknown --x86-branches-within-32B-boundaries %s | llvm-objdump -d  - | FileCheck %s
----------------
MaskRay wrote:
> If you want to test that `--x86-branches-within-32B-boundaries` expands to the 3 other `--x86-*` options.
> 
> ```
> ... -o %t
> ... -o %t2
> cmp %t %t2
> FileCheck --input-file %t
> ```
We need a file-level comment describing the purpose of the test.

What do `1a`, `2a`, and `5a` mean? If we can find appropriate, descriptive names, use them. Don't simply copy the binutils tests. If tests exist for each of the used instruction prefix, write a comment to make that clear.


================
Comment at: llvm/test/MC/X86/i386-align-branch-6a.s:10
+# CHECK:        2: 89 e5                        	movl	%esp, %ebp
+# CHECK:        4: 90                           	nop
+# CHECK:        5: 90                           	nop
----------------
Use something like `# CHECK-COUNT-20:    90  nop` (address is intentionally omitted) to skip a large number of NOPs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70157