[PATCH] D70157: Align branches within 32-Byte boundary
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 4 16:32:01 PST 2019
MaskRay added a comment.
I am still trying to understand the patch. Just made some comments about the tests.
================
Comment at: llvm/include/llvm/MC/MCFragment.h:663
+ enum SubType : uint8_t {
+ // BranchPadding - The variable size fragment to insert NOP before branch.
+ BranchPadding,
----------------
Don’t duplicate function or class name at the beginning of the comment (`BranchPadding - `). (ref: https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments)
================
Comment at: llvm/include/llvm/MC/MCFragment.h:674
+ FusedJccPadding,
+ // HardCodeBegin - The zero size fragment to mark the begin of the sequence
+ // of hard code
----------------
Full stop.
================
Comment at: llvm/include/llvm/MC/MCFragment.h:709
+ switch (SubKind) {
+ default:
+ llvm_unreachable("Unknown subtype of MCMachineDependentFragment");
----------------
Move llvm_unreachable below the switch, otherwise clang will give a warning:
warning: default label in switch which covers all enumera
tion values [-Wcovered-switch-default]
Unfortunately all GCC (even 9) -Wall will warn `warning: control reaches end of non-void function [-Wreturn-type]` unless you place an unreachable statement.
================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:537
+ // Linker may rewrite the instruction with variant symbol operand.
+ if(hasVariantSymbol(Inst)) return false;
+
----------------
Space after `if`
================
Comment at: llvm/test/MC/X86/x86-64-align-branch-1a.s:1
+# Check option --x86-branches-within-32B-boundaries is equivalent to the combination of options --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc+jmp --x86-align-branch-prefix-size=5
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown --x86-branches-within-32B-boundaries %s | llvm-objdump -d - > %t
----------------
1a~1g use the same source file. Move the source to `Inputs/align-branch-1-64.s`
According to the local naming convention, this test should probably be renamed to `align-branch-1-64.s`
================
Comment at: llvm/test/MC/X86/x86-64-align-branch-1a.s:7
+
+# CHECK: file format {{.*}}
+
----------------
Delete
================
Comment at: llvm/test/MC/X86/x86-64-align-branch-1a.s:11
+
+# CHECK: Disassembly of section .text:
+# CHECK: 0000000000000000 foo:
----------------
Delete `Disassembly of section .text:`. Ditto below.
================
Comment at: llvm/test/MC/X86/x86-64-align-branch-1b.s:4
+
+# CHECK: file format {{.*}}
+
----------------
Delete
================
Comment at: llvm/test/MC/X86/x86-64-align-branch-1b.s:10
+# CHECK: 0000000000000000 foo:
+# CHECK-NEXT: 0: 64 89 04 25 01 00 00 00 movl %eax, %fs:1
+# CHECK-NEXT: 8: 2e 55 pushq %rbp
----------------
I think 1a.s and 1b.s should be merged. FileCheck supports
--check-prefixes=CHECK,PREFIX5
--check-prefixes=CHECK,PREFIX1
```
CHECK: common part
CHECK-NEXT: common part
PREFIX5:
PREFIX5-NEXT:
PREFIX1:
PREFIX1-NEXT:
CHECK:
CHECK-NEXT:
```
```
% diff -U1 x86-64-align-branch-1[ab].s
# CHECK: 0000000000000000 foo:
-# CHECK-NEXT: 0: 64 64 64 64 89 04 25 01 00 00 00 movl %eax, %fs:1
-# CHECK-NEXT: b: 55 pushq %rbp
-# CHECK-NEXT: c: 55 pushq %rbp
-# CHECK-NEXT: d: 55 pushq %rbp
+# CHECK-NEXT: 0: 64 89 04 25 01 00 00 00 movl %eax, %fs:1
+# CHECK-NEXT: 8: 2e 55 pushq %rbp
+# CHECK-NEXT: a: 2e 55 pushq %rbp
+# CHECK-NEXT: c: 2e 55 pushq %rbp
# CHECK-NEXT: e: 48 89 e5 movq %rsp, %rbp
```
Is there performance benefit to add 4 prefixes to the same instruction?
================
Comment at: llvm/test/MC/X86/x86-64-align-branch-1c.s:2
+# Check only fused conditional jumps and conditional jumps are aligned with option --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc --x86-align-branch-prefix-size=5
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc --x86-align-branch-prefix-size=5 %s | llvm-objdump -d - | FileCheck %s
+
----------------
The difference between 1a and 1c is that 1c does not allow "jmp", but in 1a no jmp instructions get a prefix in the test, so it is unclear why 1c has different output.
================
Comment at: llvm/test/MC/X86/x86-64-align-branch-1d.s:4
+
+# CHECK: file format {{.*}}
+
----------------
Delete
================
Comment at: llvm/test/MC/X86/x86-64-align-branch-1e.s:46
+# CHECK-NEXT: 5c: eb 27 jmp {{.*}}
+# CHECK-NEXT: 5e: 90 nop
+# CHECK-NEXT: 5f: 90 nop
----------------
This is weird. Comparing this with 1d, 1e allows more instruction types, yet it inserts two NOPs which actually seems to degrade performance.
================
Comment at: llvm/test/MC/X86/x86-64-align-branch-1f.s:8
+
+# CHECK: Disassembly of section .text:
+# CHECK: 0000000000000000 foo:
----------------
No disassembly is needed. Just check that `--x86-align-branch-boundary=0` and the default (no x86- specific options) have the identical output (`cmp %t %t2`)
================
Comment at: llvm/test/MC/X86/x86-64-align-branch-1g.s:1
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown --x86-align-branch-boundary=32 -mcpu=x86-64 --x86-align-branch=jcc+jmp --x86-align-branch-prefix-size=5 %s | llvm-objdump -d - | FileCheck %s
+
----------------
Merge 1e and 1g. State that `-mcpu=x86-64` generates `66 90` instead of `90 90` (but why?)
================
Comment at: llvm/test/MC/X86/x86-64-align-branch-1g.s:3
+
+# CHECK: file format {{.*}}
+
----------------
Delete
================
Comment at: llvm/test/MC/X86/x86-64-align-branch-1g.s:7
+
+# CHECK: Disassembly of section .text:
+# CHECK: 0000000000000000 foo:
----------------
Delete `Disassembly of section .text:`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70157/new/
https://reviews.llvm.org/D70157
More information about the llvm-commits
mailing list