[llvm-dev] Preparing BOLT for LLVM monorepo

Tue Jan 11 02:33:01 PST 2022

On Mon, Jan 10, 2022 at 11:08 PM Maksim Panchenko <maks at fb.com> wrote:

> Thank you, everyone, for the review!
>
>
>
> If there are no further objections, we’d like to proceed with the merge.
>
>
>
> As Rafael posted earlier, the preview is available at
> https://github.com/facebookincubator/BOLT.
>
>
>
> For the merge, I’ve created a special branch “bolt-project-only” at
> https://github.com/facebookincubator/BOLT/tree/bolt-project-only. This
> branch contains only “/bolt” project. All commits in this branch touch just
> “/bolt”.
>
> To merge BOLT, we have to execute the following commands from the LLVM
> monorepo:
>
>   $ cd llvm-project
>
>   $ git remote add bolt-project git at github.com:facebookincubator/BOLT.git
>
>   $ git fetch bolt-project bolt-project-only
>
>   $ git pull
>
>   $ git merge --allow-unrelated-histories bolt-project/bolt-project-only
> -m "Merge BOLT into LLVM monorepo"
>
>   $ git remote remove bolt-project
>
> Then push to GH. I think we have to disable triggers (email, buildbots)
> before the push, but I’m not sure how merge commits are treated.
>
>
>
> Does this make sense and look similar to how flang and MLIR were merged?
>

This looks right (I haven't tried locally though).

Something that is worth doing before pushing is also to repack the repo
locally: it can impact the future clone size in a non-negligible way (~15%
smaller when we repacked before pushing the MLIR merge, but not much for
Flang, so your mileage may vary...).

Cheers,

-- 
Mehdi

>
>
> Thanks,
>
> Maksim
>
>
>
>
>
> On 1/10/22, 6:22 PM, "llvm-dev" <llvm-dev-bounces at lists.llvm.org> wrote:
>
>
>
> Yes, I am supportive for the merge and those are only suggestions and are
> not the block issues for the merge.
>
>
>
> See more comments below.
>
>
>
> Best
>
> Shengchen
>
> *From:* Amir Aupov <amir.aupov at gmail.com>
> *Sent:* Tuesday, January 11, 2022 6:24 AM
> *To:* Craig Topper <craig.topper at gmail.com>
> *Cc:* Kan, Shengchen <shengchen.kan at intel.com>; llvm-dev at lists.llvm.org
> *Subject:* Re: [llvm-dev] Preparing BOLT for LLVM monorepo
>
>
>
> Thanks for clarifying, Craig!
>
> +Shengchen:
>
>
>
> It looks like for the rest of the suggestions we'd need to make changes to
> tablegen.
>
> We will address them after the merge as we will have a use case in
> monorepo.
>
>
>
> Just to clarify: our understanding is that you support the merge but just
> have suggestions.
>
> We don't want to be blocked right now as we've started the merge process
> this week.
>
> Does that sound right to you?
>
>
>
> On Mon, Jan 10, 2022 at 2:02 PM Craig Topper <craig.topper at gmail.com>
> wrote:
>
>
>
> On Mon, Jan 10, 2022 at 1:12 PM Amir Aupov via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
> Hi Shengchen,
>
>
>
> Thank you for the code review and your suggestions!
>
> We've addressed some of them (see inline) but still in the process of
> addressing them all.
>
>
>
> On Tue, Jan 4, 2022 at 7:56 PM Kan, Shengchen <shengchen.kan at intel.com>
> wrote:
>
> Hi Amir
>
>
>
> I read the MC-related code and it looks fine to me in general. I’m also
> supportive for the merge. The following are some suggestions.
>
>
>
>    1. https://llvm.org/docs/CodingStandards.html#braced-initializer-lists
>     If you use a braced initializer list when initializing a variable, use an
>    equals before the open curly brace. e.g BinaryBasicBlock.h
>
> Addressed, soon to be published.
>
>
>
>
>    1.
>    https://llvm.org/docs/ProgrammersManual.html#fine-grained-debug-info-with-debug-type-and-the-debug-only-option
>    DEBUG_TYPE should not be target specific,
>
> e.g, the definition of DEBUG_TYPE should be same in X86MCPlusBuilder.cpp
> and AArch64MCPlusBuilder.cpp
>
> Addressed in
> https://github.com/facebookincubator/BOLT/commit/7ca0c5a26714754ec1dd72576ddecaa8b459842c
>
>
>
>    1. Functions getShortArithOpcode, getShortBranchOpcode in
>    X86MCPlusBuilder.cpp use reversed mappings as functions
>    getRelaxedOpcodeBranch, getRelaxedOpcodeArith in X86AsmBackend.cpp. Writing
>    twice in two places makes the code difficult to maintain, could we generate
>    the table with tablgen? The memory folding and unfolding tables in
>    X86InstrFoldTables.cpp give an example.
>
> To make sure I get it right: the tables in X86InstrFoldTables.cpp were
> generated with tablegen at some point, but are checked in as source code?
> Is there a way to rebuild these tables, so I can use that as an example?
>
>
>
> There is a tablegen backend that can generate it, but it has some bugs.
> Especially around some oddities with the SSE movd/movq instructions if I
> remember right. I used to run it after adding new instructions and picked
> the table entries I thought were ok. The code is in llvm/utils/TableGen/X86FoldTablesEmitter.cpp
> but I'm not sure how helpful it is. It's mostly trying to find pairs of
> instructions that different in the register vs memory form of the modrm
> byte with rest of the encoding being the same. For your case, the
> instructions differ in the opcode byte. I guess you'd have to write a
> tablegen backend that knew rules like if opcode==0x81 or 0x83, the other
> instruction is the one with the other opcode, but rest of the encoding is
> the same.
>
>
>
> I think we can just check the instruction name rather than opcode in the
> tablgen, e,g. ADD32ri and ADD32ri8 have the same prefix “ADD32ri”, but one
> of them has suffix “8”.  If you do not want to hand-write the table rather
> than generate it w/ tablgen, at least getShortArithOpcode can share the
> same table with getRelaxedOpcodeArith. The function pair lookupUnfoldTable
> and lookupFoldTable is an example.
>
>
>
>
>    1. It would better if functions isADD, isAND, isCMP, isDEC, isINC,
>    isSUB and isTEST can be generated with tablgen.
>
> Removed our definitions of isINC/isDEC:
> https://github.com/facebookincubator/BOLT/commit/9fec2d58964d643a7436ba2804f8fb967f23881e
>
> How do we approach generating isADD/... with tablegen? There are
> appropriate definitions (`defm ADD`, etc)
> in llvm/lib/Target/X86/X86InstrArithmetic.td but I'm not sure how to get
> tablegen to generate the tables.
>
>
>
> I think you'd have to add isAdd, isCmp, etc. fields to the X86Inst class
> and set them on the appropriate instructions. Then have a tablegen backend
> collect all the instructions with each property. Or you could figure out
> the different encoding rules like if the opcode is 0x80-0x83 than the
> format being MRM4r/MRM4m/MRM4X mean the opcode is an AND.
>
>
>
> I think we can just check the instruction name in tablgen, e.g, ADD8ri,
> ADD16ri, ADD32ri, ADD32ri8 and ADD64ri have same prefix “ADD” followed by a
> number .
>
>
>
> Thanks,
>
> -Amir
>
>
>
>
>
> Best
>
> Shengchen
>
> *From:* llvm-dev <llvm-dev-bounces at lists.llvm.org> *On Behalf Of *Wang,
> Phoebe via llvm-dev
> *Sent:* Thursday, December 30, 2021 5:33 PM
> *To:* Amir Aupov <amir.aupov at gmail.com>
> *Cc:* llvm-dev at lists.llvm.org
> *Subject:* Re: [llvm-dev] Preparing BOLT for LLVM monorepo
>
>
>
> Hi Amir,
>
>
>
> Amazing job! Thanks for the quick and perfect changes. I don’t have any
> other problems now. Hoping seeing the code in main trunk soon.
>
>
>
> Thanks
>
> Phoebe
>
>
>
> *From:* Amir Aupov <amir.aupov at gmail.com>
> *Sent:* Thursday, December 30, 2021 4:32 PM
> *To:* Wang, Phoebe <phoebe.wang at intel.com>
> *Cc:* llvm-dev at lists.llvm.org
> *Subject:* Re: [llvm-dev] Preparing BOLT for LLVM monorepo
>
>
>
> Hi Phoebe,
>
>
>
> Thank you again for reviewing the codebase and for your feedback. We've
> addressed it in recent commits:
>
> >  1.  Use the monorepo of LLVM in the example for convenience in https://github.com/facebookincubator/BOLT/blob/main/bolt/docs/OptimizingClang.md#getting-clang-7-sources
>
> E.g, git clone --branch=release/7.x https://github.com/llvm/llvm-project.git
>
> https://github.com/facebookincubator/BOLT/commit/c4ed6a11eb264660f5cfe7f5165df35b63d52608
>
>
>
> >  1.  I found there are 142 code with “not implemented”, most of which are in Core/MCPlusBuilder.h.
>
>      *   Do they affect the functionality of BOLT?
>
>      *   Do you have plan to implement them recently or can they be removed instead?
>
> Explained above.
>
> >  2.  I noticed some inconsistent use of braces in the code. Maybe better to follow with LLVM coding standard<https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements>.
>
>      *   https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/BinaryBasicBlock.cpp#L241, L289<https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/BinaryBasicBlock.cpp#L289>, etc.
>
>      *   https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/DebugData.cpp#L193
>
> https://github.com/facebookincubator/BOLT/commit/fe3fcf094e320af4da4a98a5387820cd0ca826b9
>
> https://github.com/facebookincubator/BOLT/commit/b40699ed90882465e49ec46e4d3ad28623e54c69
>
> https://github.com/facebookincubator/BOLT/commit/c21ee425083276a7af700e232df15e2a3d31dacb
>
> https://github.com/facebookincubator/BOLT/commit/ebc06afcfc35fbb4360b3788adeb6228c27ad830
>
> https://github.com/facebookincubator/BOLT/commit/dca3003fce711daee387bbbf66bf4ecbe1b9ab9b
>
> https://github.com/facebookincubator/BOLT/commit/cd526a4550455b0f203771088cef6e512b0dd1a2
>
> >  3.  Some files don’t have a descriptions in the first line, e.g.  DynoStats.cpp<https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/DynoStats.cpp#L1>, ParallelUtilities.cpp<https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/ParallelUtilities.cpp#L1>, etc.
>
> >  4.  Leaving without descriptions might be fine, but the format should be consistent. Leaving with spaces like in BinaryFunctionProfile.cpp<https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/BinaryFunctionProfile.cpp#L1> doesn’t make sense.
>
>
> https://github.com/facebookincubator/BOLT/commit/349c8abc696919e989418895f4ade24a4c910617
>
>
>
> On Tue, Dec 21, 2021 at 4:22 AM Wang, Phoebe <phoebe.wang at intel.com>
> wrote:
>
> Hi Amir,
>
>
>
> It makes sense to me. Thanks!
>
>
>
> *From:* llvm-dev <llvm-dev-bounces at lists.llvm.org> *On Behalf Of *Amir
> Aupov via llvm-dev
> *Sent:* Tuesday, December 21, 2021 8:57 AM
> *To:* llvm-dev at lists.llvm.org
> *Subject:* Re: [llvm-dev] Preparing BOLT for LLVM monorepo
>
>
>
> Hi Phoebe,
>
>
>
> Thanks again for taking the time to look at the codebase. We've added your
> feedback to our upstreaming tracking issue on github:
> https://github.com/facebookincubator/BOLT/issues/248. We've started
> addressing the issues.
>
>
>
> Regarding "not implemented” in Core/MCPlusBuilder.h: it's a design
> decision to prevent the use of non-implemented target-specific methods
> (these are virtual methods). Target-specific implementation goes to
> X86MCPlusBuilder and AArch64MCPlusBuilder. Some methods only make sense for
> specific targets, so we intend to keep llvm_unreachable statements in
> MCPlusBuilder.h.
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20220111/84ff570d/attachment-0001.html>