[PATCH] D70157: Align branches within 32-Byte boundary

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

I've been digging through the code for this for the last day or so.  This is a new area for me, so it's possible I'm off base, but I have some concerns about the current design.

First, there appears to already be support for instruction bundling and alignment in the assembler today.  I stumbled across the .bundle_align_mode, .bundle_start, and .bundle_end mechanism (https://lists.llvm.org/pipermail/llvm-dev/2012-December/056723.html) which seems to *heavily* overlap with this proposal.  I suspect that the compiler support suggested by James and myself earlier in this thread could be implemented on to this existing mechanism.

Second, the new callbacks and infrastructure added appear to overlap heavily w/the MCCodePadding infrastructure.  (Which, admitted, appears unused and untested.)

Third, I have not see a justification for why complexity for instruction prefix padding is necessary.  All the effected CPUs support multi-byte nops, so we're talking about a *single micro op* difference between the nop form and prefix form.  Can anyone point to a performance delta due to this?  If not, I'd suggest we should start with the nop form, and then build the prefix form in a generic manner for all alignment varieties.


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

https://reviews.llvm.org/D70157