[PATCH] D72225: Align branches within 32-Byte boundary(Prefix padding)

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

I'm having a really hard time wrapping my head around the implementation.  Can you give a high level summary of the usage of BoundaryAlign after this change?  A couple of examples w/all the fragments written out might also help a lot.



================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:477
+  // is asked to be aligned.
+  const MCInstrDesc &InstDesc = MCII->get(Inst.getOpcode());
+  if (InstDesc.isBranch() || InstDesc.isCall() || InstDesc.isReturn())
----------------
This should probably be:
if (needAlign(Inst))
  return false;


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:560
+  // since there is no clear instruction boundary.
+  if (isa_and_nonnull<MCDataFragment>(CF) && !CF->hasInstructions())
+    return;
----------------
Please remove this.  It should be covered by the compiler support patch which already landed for compiled code, and he assembler syntax is separate.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:569
+    OS.getAssembler().getEmitter().emitPrefix(Inst, VecOS, STI);
+    uint8_t ExistingPrefixSize = static_cast<uint8_t>(Code.size());
+    return (AlignMaxPrefixSize > ExistingPrefixSize)
----------------
Please assert that the total number of prefixes fits within a uint8_t.  It does, but having that explicitly asserted/noted would be helpful.


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

https://reviews.llvm.org/D72225