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

annita.zhang added a comment. In D70157#1793280 , @reames wrote: > I've gone ahead and landed the patch so that we can iterate in tree. See > commit 14fc20ca62821b5f85582bf76a467d412248c248 >

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

MaskRay added a comment. In D70157#1793280 , @reames wrote: > I've gone ahead and landed the patch so that we can iterate in tree. See > commit 14fc20ca62821b5f85582bf76a467d412248c248 >

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

reames added a comment. I've gone ahead and landed the patch so that we can iterate in tree. See commit 14fc20ca62821b5f85582bf76a467d412248c248 . I've also landed a couple of follow up patches to address issues which would

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

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG14fc20ca6282: Align branches within 32-Byte boundary (NOP padding) (authored by reames). Repository: rG LLVM Github

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

skan updated this revision to Diff 234846. skan added a comment. 1. Add more tests for `!VK_NONE` cases. 2. Reduce pervasive `auto` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70157/new/ https://reviews.llvm.org/D70157 Files: llvm/include/llvm/MC/MCAsmBackend.h

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

MaskRay added a comment. In D70157#1792262 , @skan wrote: > Do you think this patch is ready to land? @MaskRay It is 00:35 here and I should confess that I haven't read through this yet. There are some minor things like (1) pervasive `auto` (2)

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

skan added a comment. Do you think this patch is ready to land? @MaskRay CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70157/new/ https://reviews.llvm.org/D70157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

MaskRay added inline comments. Comment at: llvm/lib/MC/MCAssembler.cpp:1003 + // exists) also marks the end of the branch. + for (auto i = 0U, N = BF.isFused() ? 2U : 1U; + i != N && !isa(F); ++i, F = F->getNextNode()) { skan wrote: > MaskRay wrote: > >

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

skan updated this revision to Diff 234830. skan added a comment. 1. Simplify the test cases. 2. Add some comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70157/new/ https://reviews.llvm.org/D70157 Files: llvm/include/llvm/MC/MCAsmBackend.h llvm/include/llvm/MC/MCAssembler.h

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

skan marked an inline comment as done. skan added inline comments. Comment at: llvm/lib/MC/MCAssembler.cpp:1003 + // exists) also marks the end of the branch. + for (auto i = 0U, N = BF.isFused() ? 2U : 1U; + i != N && !isa(F); ++i, F = F->getNextNode()) {

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

MaskRay added inline comments. Comment at: llvm/lib/MC/MCAssembler.cpp:993 + MCBoundaryAlignFragment ) { + // The MCBoundaryAlignFragment that doesn't emit NOP should not relax. + if (!BF.canEmitNops()) be relaxed

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

skan added a comment. In D70157#1791347 , @reames wrote: > The general question is why a *range* of fragments can't be defined. > Computing the instruction size for the entire range then just requires > walking from first to last fragment in the range

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

annita.zhang added a comment. > To be concrete, I propose: > ".autopad", ".noautopad": allow/disallow the assembler to emit padding via > inserting a nop or prefix before any instruction, as needed. > ".align_branch_boundary [N,] [instruction,]": Enable branch-boundary padding > (per previous

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

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

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

reames added a comment. In D70157#1789159 , @skan wrote: > In D70157#1788445 , @reames wrote: > > > Specifically on the revised patch, I remain confused by the need for > > multiple subtypes. The need for

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

skan updated this revision to Diff 234697. skan added a comment. Add more comment CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70157/new/ https://reviews.llvm.org/D70157 Files: llvm/include/llvm/MC/MCAsmBackend.h llvm/include/llvm/MC/MCAssembler.h

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

skan updated this revision to Diff 234650. skan added a comment. move the code that checks if we can reuse the current `MCBoundaryAlignFragment` into the function `X86AsmBackend::getOrCreateBoundaryAlignFragment` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70157/new/

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

skan updated this revision to Diff 234520. skan added a comment. Fix a typo in `MCFragment::dump()` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70157/new/ https://reviews.llvm.org/D70157 Files: llvm/include/llvm/MC/MCAsmBackend.h llvm/include/llvm/MC/MCAssembler.h

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

skan updated this revision to Diff 234515. skan added a comment. 1. rename `MCBoundaryAlignFragment::hasEmitNop()` to `MCBoundaryAlignFragment::canEmitNop()` 2. reduce the number of `MCBoundaryAlignFragment` emitted as possible CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70157/new/

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

skan updated this revision to Diff 234488. skan edited the summary of this revision. skan added a comment. **Simplify** Drop the subtype of `MCBoundaryAlignFragment` and add data member `EmitNops` to indicate whether NOPs should be emitted. CHANGES SINCE LAST ACTION

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

skan added a comment. In D70157#1788445 , @reames wrote: > Specifically on the revised patch, I remain confused by the need for multiple > subtypes. The need for fragments *between* the potentially fused > instructions doesn't make sense to me. What I

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

reames added a comment. I've updated the draft assembler support in https://reviews.llvm.org/D71315 to match the proposal here. Again, this is very much WIP and mostly just to show proposed syntax/usage. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70157/new/

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

jyknight added a comment. In D70157#1788418 , @reames wrote: > In D70157#1788025 , @jyknight wrote: > > > > .push_align_branch_boundary [N,] [instruction,]* > > > > I'd like to raise again the possibility of using

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

reames added a comment. Specifically on the revised patch, I remain confused by the need for multiple subtypes. The need for fragments *between* the potentially fused instructions doesn't make sense to me. What I was expecting to see was the following: BoundaryAlign w/target=the branch

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

reames added a comment. In D70157#1788025 , @jyknight wrote: > > .push_align_branch_boundary [N,] [instruction,]* > > I'd like to raise again the possibility of using a more general region > directive to denote "It is allowable to add prefixes/nops

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

reames added a comment. In D70157#1787403 , @annita.zhang wrote: > In D70157#1787160 , @MaskRay wrote: > > > > > > > > > There is a precedant: .pushsection/.popsection (MCStreamer::SectionStack). > > With

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

skan updated this revision to Diff 234312. skan retitled this revision from "Align branches within 32-Byte boundary" to "Align branches within 32-Byte boundary(NOP padding)". skan edited the summary of this revision. skan added a comment. **Simplify** 1. Drop prefix padding support 2. Drop