[PATCH] D72303: [BranchAlign] Compiler support for suppressing branch align

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Generally good, only some suggestions for the tests.



================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1150
+  bool OldAllowAutoPadding;
+  NoAutoPaddingScope(MCStreamer &OS)
+    : OS(OS) {
----------------
We could also be more specific. As an example, disabling auto padding only when `--x86-align-branch=jcc` is included & jcc is emitted by the relevant LowerXXXXX. The value is low, so being unconditional should be fine.


================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1285
   // PATCHABLE_OP minsize, opcode, operands
+  
+  NoAutoPaddingScope NoPadScope(*OutStreamer);
----------------
Whitespace on an otherwise empty line.

Some other places in this file has such whitespace.


================
Comment at: llvm/test/CodeGen/X86/align-branch-boundary-noautopadding.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -O3 -mcpu=skylake -x86-align-branch-boundary=32 -x86-align-branch=call -filetype=obj < %s | llvm-objdump -d --no-show-raw-insn - | FileCheck %s
----------------
This should be unnecessary.


================
Comment at: llvm/test/CodeGen/X86/align-branch-boundary-noautopadding.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -O3 -mcpu=skylake -x86-align-branch-boundary=32 -x86-align-branch=call -filetype=obj < %s | llvm-objdump -d --no-show-raw-insn - | FileCheck %s
+
----------------
Make `llvm-objdump | FileCheck` a separate step.

Add a separate assembly output test, checking `#noautopadding` is emitted.


================
Comment at: llvm/test/CodeGen/X86/align-branch-boundary-noautopadding.ll:18
+  ; happen for the safepoint.  That's non-ideal, we'd really prefer to do
+  ; the alignment and just keep the label with the statepoint call. (TODO)
+  call void @foo()
----------------
Just check the intended offset sequence.

```
0:
1: callq
6: callq
b: callq
10: callq
15: callq
1a: callq
1f: callq
```


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

https://reviews.llvm.org/D72303