[PATCH] D70157: Align branches within 32-Byte boundary(NOP padding)

[Philip Reames via Phabricator via cfe-commits]

reames accepted this revision.
reames added a comment.

LGTM.  The patch isn't perfect, but this has reached the point where landing and iterating in tree is the fastest way to convergence.  To be clear, this LGTM *only* applies to the current patch contents, and the *internal only* flag names this introduces.  These may change before we expose this publicly.

Warts with the current patch we should iterate to address:

- Stylistic issues w.r.t. comments, naming, static vs member functions remain.  None are show stoppers.
- Many of the tests can probably be simplified and condensed.
- The bundling scheme used is probably more complicated than needed (see previous suggestions).
- This patch doesn't include anyway to locally disable padding, and thus is *known incorrect* in some cases.  As this remains off by default, this is not a blocker for commit.

p.s. I spoke w/James this morning, and we came up with some revisions in approach he's going to suggest separately.  We did explicitly discuss the status of the current patch, and whether it needed further review cycles or could be iterated in tree.  Normally, I'd wait for him to summarize that conversation publicly, but given the time delay and vacation schedules involved, I want to avoid loosing another day cycle.



================
Comment at: llvm/include/llvm/MC/MCFragment.h:570
+private:
+  /// The size of the MCBoundaryAlignFragment.
+  uint64_t Size = 0;
----------------
Please add a note that the size is lazily set during relaxation, and is not meaningful before that.  


================
Comment at: llvm/test/MC/X86/align-branch-32-1a.s:34
+foo:
+  movl  %eax, %fs:0x1
+  pushl  %ebp
----------------
For testing alignment functionality, we can probably use a repl int3 pattern here.  It would make the tests much more concise, and shouldn't effect the logic being (currently) tested.  



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

https://reviews.llvm.org/D70157