Wang, Phoebe via llvm-dev
2021-Dec-21 12:22 UTC
[llvm-dev] Preparing BOLT for LLVM monorepo
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/20211221/38ca54f1/attachment.html>
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-sourcesE.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>