[llvm-dev] Preparing BOLT for LLVM monorepo

[Wang, Phoebe via llvm-dev]

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<mailto:phoebe.wang at intel.com>> wrote:
Hi Amir,

It makes sense to me. Thanks!

From: llvm-dev <llvm-dev-bounces at lists.llvm.org<mailto: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<mailto: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/f878311f/attachment.html>