[llvm-dev] Preparing BOLT for LLVM monorepo

Amir Aupov via llvm-dev llvm-dev at lists.llvm.org
Thu Dec 30 00:32:27 PST 2021

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


>  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,
     *   https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/DebugData.cpp#L193






>  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.


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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20211230/df6f294a/attachment.html>

More information about the llvm-dev mailing list