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

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D70157#1787403 <https://reviews.llvm.org/D70157#1787403>, @annita.zhang wrote:

> In D70157#1787160 <https://reviews.llvm.org/D70157#1787160>, @MaskRay wrote:
>
> >
>
>
>
>
> > There is a precedant: .pushsection/.popsection (MCStreamer::SectionStack). With .push_align_branch/.pop_align_branch, we probably don't need the 'switch-to-default' action.
> > 
> > I don't know how likely we may ever need nested states (e.g. an `.include` directive inside an .align_branch region where the included file has own idea about branch alignment), but .push/.pop does not seem to be more complex than disable/enable/default.
>
> I rethink about the directives and prefer the .push/.pop pair as @MaskRay suggested. To be specified, I'd suggest to use .push_align_branch_boundary and .pop_align_branch_boundary to align with MC command line options. They will cowork with the command line options and overwrite the options if both are existing.


I agree that we need the push/pop semantics.

> To be clarified, I described the behavior of the directives from my understanding. Feel free to speak if you have difference opinion.
> 
> .push_align_branch_boundary [N,] [instruction,]*
> 
>   This directive specifies the beginning of a region which will overwrite the value set by the command line or by the previous directive. It can represent either an enabling or disabling directive controlled by parameter N. 
>   N indicates to align the branches within N byte boundary. The default value is 32. If N is 0, it means the branch alignment is off within this region. 
>   Instruction specifies types of branches to align. The value is one or multiple values from fused, jcc, jmp, call, ret and indirect. The default value is fused, jcc and jmp. (may change later)

I'd remove the defaults.  Let's just be explicit about what is being enabled/disabled.


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

https://reviews.llvm.org/D70157