Kan, Shengchen via llvm-dev
2022-Jan-05 03:56 UTC
[llvm-dev] Preparing BOLT for LLVM monorepo
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 2. 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 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. 2. It would better if functions isADD, isAND, isCMP, isDEC, isINC, isSUB and isTEST can be generated with tablgen. 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<mailto:amir.aupov at gmail.com>> Sent: Thursday, December 30, 2021 4:32 PM To: Wang, Phoebe <phoebe.wang at intel.com<mailto:phoebe.wang at intel.com>> Cc: llvm-dev at lists.llvm.org<mailto: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-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<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/20220105/ac1a3e85/attachment.html>
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 weregenerated 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?> 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. 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. > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20220110/fe9e81a8/attachment.html>