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

annita.zhang added a comment. In D70157#1744243 , @chandlerc wrote: > Thanks for sending this out! > > I've made a minor comment on the flag structure, but my bigger question would > be: can you summarize the performance hit and code size hit you've seen

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

annita.zhang added a comment. In D70157#1768319 , @chandlerc wrote: > I'm seeing lots of updates to fix bugs, but no movement for many days on both > my meta comments and (in some ways more importantly) James's meta comments. > (And thanks Philip for

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

annita.zhang added a comment. > The point is that we have explicit requirement at the start and we have a > lowering into 16-byte sequence that we need to be preserved exactly as it is. > Essentially what we need is a "protection" for this sequence from any > changes by machinery that

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

annita.zhang added a comment. In 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. >

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

annita.zhang added a comment. In D70157#1787139 , @reames wrote: > Here are the minutes from our phone call a few minutes ago. Thanks for coordinating the meeting and having a clear summary. It helps a lot to accelerate the patch review. I really

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

annita.zhang added a comment. In D70157#1760278 , @jyknight wrote: > > Branch alignment > > --- > > The primary goal of this patch, restricting the placement of branch > instructions, is a performance optimization. Similar to loop alignment, the

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

annita.zhang added a comment. > > >>> 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.

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

annita.zhang added a comment. In D70157#1768338 , @annita.zhang wrote: > In D70157#1768319 , @chandlerc wrote: > > > I'm seeing lots of updates to fix bugs, but no movement for many days on > > both my meta

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

annita.zhang added a comment. > More performance data was posted on > http://lists.llvm.org/pipermail/llvm-dev/2019-December/137609.html and > http://lists.llvm.org/pipermail/llvm-dev/2019-December/137610.html. Let's > move on based on the data. Based on the SPEC data, we observed 2.6% and

[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)

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] D75017: [Driver][X86] Add helptext for malign-branch*, mbranches-within-32B-boundaries

annita.zhang added inline comments. Comment at: clang/include/clang/Driver/Options.td:2175 +def mbranches_within_32B_boundaries : Flag<["-"], "mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group, + HelpText<"Align branches within 32-byte boundary">; def

[PATCH] D75017: [Driver][X86] Add helptext for malign-branch*, mbranches-within-32B-boundaries

annita.zhang accepted this revision. annita.zhang added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75017/new/ https://reviews.llvm.org/D75017 ___ cfe-commits mailing list

[PATCH] D72463: [Driver][X86] Add -malign-branch* and -mbranches-within-32B-boundaries

annita.zhang added a comment. In D72463#1826821 , @MaskRay wrote: > In D72463#1826499 , @annita.zhang > wrote: > > > @MaskRay Did you merge it to LLVM 10 branch? > > > It is included in the release branch. > >

[PATCH] D72463: [Driver][X86] Add -malign-branch* and -malign-branch-within-32B-boundaries

annita.zhang added a comment. @MaskRay Did you merge it to LLVM 10 branch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72463/new/ https://reviews.llvm.org/D72463 ___ cfe-commits mailing list

[PATCH] D72227: Add options for clang to align branches within 32B boundary

annita.zhang added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2052 +CmdArgs.push_back("-mllvm"); +CmdArgs.push_back(Args.MakeArgString("-x86-align-branch-prefix-size=4")); + }

[PATCH] D72227: Add options for clang to align branches within 32B boundary

annita.zhang added inline comments. Comment at: clang/include/clang/Driver/Options.td:2198 + "should be a power of 2 and no less than 32. Branches will be " + "aligned within the boundary of specified size. " +

[PATCH] D72227: Add options for clang to align branches within 32B boundary

annita.zhang requested changes to this revision. annita.zhang added a comment. This revision now requires changes to proceed. A general comment. All the tests have been done with -malign-branch-prefix-size=5. I don't see any explicit reason to change the default prefix size from 5 to 4.