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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
