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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

I've addressed the vast majority of style comments and have noted why I don't think the remainder are appropriate.



================
Comment at: llvm/lib/Target/X86/X86MCInstLower.cpp:1155-1157
+  ~NoAutoPaddingScope() {
+    changeAndComment(OldAllowAutoPadding);
+  }
----------------
skan wrote:
> keep the code in one line
This is not idiomatic for non-accessors.  


================
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:
> reames wrote:
> > 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.
> > I'd really prefer not to introduce a temporary file here. It adds no value.
> 
> OK. No strong opinion here.
On further reflection, I don't plan to add the textual assembly either.  That exact case is already covered in the other test file.  


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

https://reviews.llvm.org/D72303