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

[Philip Reames via Phabricator via llvm-commits]

reames marked 3 inline comments as done.
reames added inline comments.


================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1150
+  bool OldAllowAutoPadding;
+  NoAutoPaddingScope(MCStreamer &OS)
+    : OS(OS) {
----------------
MaskRay wrote:
> 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.
Sorry, was this a comment, or a request?  I definitely do not want to make this any more complicated at this time.  Only reason I bothered filtering the comments at all was to keep the test churn down.  


================
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
----------------
MaskRay wrote:
> This should be unnecessary.
What specifically?


================
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
+
----------------
MaskRay wrote:
> Make `llvm-objdump | FileCheck` a separate step.
> 
> Add a separate assembly output test, checking `#noautopadding` is emitted.
I'd really prefer not to introduce a temporary file here.  It adds no value.

Happy to add the textual assembly check.


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

https://reviews.llvm.org/D72303