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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Here are the minutes from our phone call a few minutes ago.

Attendees: Andy Kaylor, Craig Topper, Annita Zhang, Tom Stellard, Chandler Carruth, Fedor Sergeev, Philip Reames, Yuanake Luo

Status Summary
==============

Performance data has been posted to llvm-dev.  We had a side discussion about nop encoding, and it was mentioned these numbers were collected from runs targeting skylake (i.e. not generic x86).  This is similar to the result we (Azul) have collected and shared summaries of previously.

GNU patch has landed Friday - this mostly fixes assembler command line.

Discussion on Approach
======================

Three major options debated:

  Assembler only - as in the current patch, assembler does all work, only command line flag
  Explicit Directive only - as proposed in my alternate patch, compiler decides exactly what instructions get aligned
  Region based directives - as proposed in James' last comment on review, directives enable and disable auto-padding in assembler

Use cases identified:

  compiler users
      important than assembler is self contained (i.e. don't have to know compiler options for reproduceability)
      inline assembly looks a lot like assembler users
  legacy assembler
      important that existing assembly works unmodified
  assembler users
      "try it and see" model vs selective enable vs selective disable
      likely need to support all three

Consensus was that the region based directives met use cases the best.  In particular, desire to be able to overrule default (for say, inline assembly or a JITs patchability assumptions) and then restore default.  Default assembler behavior remains unchanged.

Stawman syntax proposal

.align_branch_boundary disable/default
.align_branch_boundary enable N, instructions (fused, jcc, jmp, etc..)

We need to ensure a consensus on syntax is shared w/gnu.  Annita agreed to coordinate this.

Compiler would essentially just wrap generated assembly in directives.

Issue noticed while writing this up: proposed syntax assumes a default has been set, but doesn't give a way to set one.  This would seem to break the desired reproducibility property for compiled code.  Revision needed.

Push/Pop semantics were suggested at one point, but were thought to be non-idiomatic?

Other Topics
============

We very quickly discussed nop vs prefix performance.  There was a clear consensus in starting with nop only patch and evolving later as needed.

Next Steps
==========

Annita will refresh current patch with two key changes.  1) Drop prefix support and simplify and 2) drop clang driver support for now.  Desire is to minimize cycle time before next iteration so that feedback on approach can be given while reviewers are still around.

Philip will prototype directive parsing support.  Annita and Yuo (??) to handle coordination on syntax.

Suggested patch split:

  (current patch) command line option to set default, nop only version w/cleaned up code as much as possible
  assembler directive support (draft by Philip in parallel)
  (future) compiler patch to wrap by default

Side note to Annita: For you to remove "hard code", you'll have to have a placeholder for the enable/disable interface.  That should probably be split and rebased in my patch.


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

https://reviews.llvm.org/D70157