[llvm-dev] Preparing BOLT for LLVM monorepo

[Amir Aupov via llvm-dev]

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