<div dir="ltr">Hi Phoebe,<div><br></div><div>Thank you again for reviewing the codebase and for your feedback. We've addressed it in recent commits:</div><div><pre style="white-space:pre-wrap;color:rgb(0,0,0)">> 1. Use the monorepo of LLVM in the example for convenience in <a href="https://github.com/facebookincubator/BOLT/blob/main/bolt/docs/OptimizingClang.md#getting-clang-7-sources">https://github.com/facebookincubator/BOLT/blob/main/bolt/docs/OptimizingClang.md#getting-clang-7-sources</a>
E.g, git clone --branch=release/7.x <a href="https://github.com/llvm/llvm-project.git">https://github.com/llvm/llvm-project.git</a></pre><pre style="white-space:pre-wrap;color:rgb(0,0,0)"><a href="https://github.com/facebookincubator/BOLT/commit/c4ed6a11eb264660f5cfe7f5165df35b63d52608">https://github.com/facebookincubator/BOLT/commit/c4ed6a11eb264660f5cfe7f5165df35b63d52608</a>
> 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?</pre><pre style="white-space:pre-wrap;color:rgb(0,0,0)">Explained above.
> 2. I noticed some inconsistent use of braces in the code. Maybe better to follow with LLVM coding standard<<a href="https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements">https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements</a>>.
* <a href="https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/BinaryBasicBlock.cpp#L241,">https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/BinaryBasicBlock.cpp#L241,</a> L289<<a href="https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/BinaryBasicBlock.cpp#L289">https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/BinaryBasicBlock.cpp#L289</a>>, etc.
* <a href="https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/DebugData.cpp#L193">https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/DebugData.cpp#L193</a></pre><pre style="white-space:pre-wrap;color:rgb(0,0,0)"><a href="https://github.com/facebookincubator/BOLT/commit/fe3fcf094e320af4da4a98a5387820cd0ca826b9">https://github.com/facebookincubator/BOLT/commit/fe3fcf094e320af4da4a98a5387820cd0ca826b9</a></pre><pre style="white-space:pre-wrap;color:rgb(0,0,0)"><a href="https://github.com/facebookincubator/BOLT/commit/b40699ed90882465e49ec46e4d3ad28623e54c69">https://github.com/facebookincubator/BOLT/commit/b40699ed90882465e49ec46e4d3ad28623e54c69</a></pre><pre style="white-space:pre-wrap;color:rgb(0,0,0)"><a href="https://github.com/facebookincubator/BOLT/commit/c21ee425083276a7af700e232df15e2a3d31dacb">https://github.com/facebookincubator/BOLT/commit/c21ee425083276a7af700e232df15e2a3d31dacb</a></pre><pre style="white-space:pre-wrap;color:rgb(0,0,0)"><a href="https://github.com/facebookincubator/BOLT/commit/ebc06afcfc35fbb4360b3788adeb6228c27ad830">https://github.com/facebookincubator/BOLT/commit/ebc06afcfc35fbb4360b3788adeb6228c27ad830</a></pre><pre style="white-space:pre-wrap;color:rgb(0,0,0)"><a href="https://github.com/facebookincubator/BOLT/commit/dca3003fce711daee387bbbf66bf4ecbe1b9ab9b">https://github.com/facebookincubator/BOLT/commit/dca3003fce711daee387bbbf66bf4ecbe1b9ab9b</a></pre><pre style="white-space:pre-wrap;color:rgb(0,0,0)"><a href="https://github.com/facebookincubator/BOLT/commit/cd526a4550455b0f203771088cef6e512b0dd1a2">https://github.com/facebookincubator/BOLT/commit/cd526a4550455b0f203771088cef6e512b0dd1a2</a>
> 3. Some files don’t have a descriptions in the first line, e.g. DynoStats.cpp<<a href="https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/DynoStats.cpp#L1">https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/DynoStats.cpp#L1</a>>, ParallelUtilities.cpp<<a href="https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/ParallelUtilities.cpp#L1">https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/ParallelUtilities.cpp#L1</a>>, etc.</pre><pre style="white-space:pre-wrap;color:rgb(0,0,0)">> 4. Leaving without descriptions might be fine, but the format should be consistent. Leaving with spaces like in BinaryFunctionProfile.cpp<<a href="https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/BinaryFunctionProfile.cpp#L1">https://github.com/facebookincubator/BOLT/blob/main/bolt/lib/Core/BinaryFunctionProfile.cpp#L1</a>> doesn’t make sense.
</pre><a href="https://github.com/facebookincubator/BOLT/commit/349c8abc696919e989418895f4ade24a4c910617">https://github.com/facebookincubator/BOLT/commit/349c8abc696919e989418895f4ade24a4c910617</a><br class="gmail-Apple-interchange-newline"></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Dec 21, 2021 at 4:22 AM Wang, Phoebe <<a href="mailto:phoebe.wang@intel.com">phoebe.wang@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div lang="EN-US" style="overflow-wrap: break-word;">
<div class="gmail-m_-8031958577465292009WordSection1">
<p class="MsoNormal"><span style="color:rgb(31,73,125)">Hi Amir,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="color:rgb(31,73,125)">It makes sense to me. Thanks!<u></u><u></u></span></p>
<p class="MsoNormal"><span style="color:rgb(31,73,125)"><u></u> <u></u></span></p>
<div style="border-right:none;border-bottom:none;border-left:none;border-top:1pt solid rgb(225,225,225);padding:3pt 0in 0in">
<p class="MsoNormal"><b>From:</b> llvm-dev <<a href="mailto:llvm-dev-bounces@lists.llvm.org" target="_blank">llvm-dev-bounces@lists.llvm.org</a>> <b>On Behalf Of
</b>Amir Aupov via llvm-dev<br>
<b>Sent:</b> Tuesday, December 21, 2021 8:57 AM<br>
<b>To:</b> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<b>Subject:</b> Re: [llvm-dev] Preparing BOLT for LLVM monorepo<u></u><u></u></p>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">Hi Phoebe,<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Thanks again for taking the time to look at the codebase. We've added your feedback to our upstreaming tracking issue on github: <a href="https://github.com/facebookincubator/BOLT/issues/248" target="_blank">https://github.com/facebookincubator/BOLT/issues/248</a>.
We've started addressing the issues.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">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.<u></u><u></u></p>
</div>
</div>
</div>
</div>
</blockquote></div>