<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:SimSun;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
@font-face
{font-family:"\@SimSun";
panose-1:2 1 6 0 3 1 1 1 1 1;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;}
span.EmailStyle20
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;
font-weight:normal;
font-style:normal;
text-decoration:none none;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal"><span style="color:#1F497D">Hi Amir,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">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.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">Thanks<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D">Phoebe<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> Amir Aupov <amir.aupov@gmail.com> <br>
<b>Sent:</b> Thursday, December 30, 2021 4:32 PM<br>
<b>To:</b> Wang, Phoebe <phoebe.wang@intel.com><br>
<b>Cc:</b> llvm-dev@lists.llvm.org<br>
<b>Subject:</b> Re: [llvm-dev] Preparing BOLT for LLVM monorepo<o:p></o:p></p>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">Hi Phoebe,<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Thank you again for reviewing the codebase and for your feedback. We've addressed it in recent commits:<o:p></o:p></p>
</div>
<div>
<pre style="white-space:pre-wrap"><span style="color:black">> 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><o:p></o:p></span></pre>
<pre><span style="color:black">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><o:p></o:p></span></pre>
<pre style="white-space:pre-wrap"><span style="color:black"><a href="https://github.com/facebookincubator/BOLT/commit/c4ed6a11eb264660f5cfe7f5165df35b63d52608">https://github.com/facebookincubator/BOLT/commit/c4ed6a11eb264660f5cfe7f5165df35b63d52608</a><o:p></o:p></span></pre>
<pre><span style="color:black"><o:p> </o:p></span></pre>
<pre><span style="color:black">> 1. I found there are 142 code with “not implemented”, most of which are in Core/MCPlusBuilder.h.<o:p></o:p></span></pre>
<pre><span style="color:black"> * Do they affect the functionality of BOLT?<o:p></o:p></span></pre>
<pre><span style="color:black"> * Do you have plan to implement them recently or can they be removed instead?<o:p></o:p></span></pre>
<pre style="white-space:pre-wrap"><span style="color:black">Explained above.<o:p></o:p></span></pre>
<pre><span style="color:black">> 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>>.<o:p></o:p></span></pre>
<pre><span style="color:black"> * <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.<o:p></o:p></span></pre>
<pre><span style="color:black"> * <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><o:p></o:p></span></pre>
<pre style="white-space:pre-wrap"><span style="color:black"><a href="https://github.com/facebookincubator/BOLT/commit/fe3fcf094e320af4da4a98a5387820cd0ca826b9">https://github.com/facebookincubator/BOLT/commit/fe3fcf094e320af4da4a98a5387820cd0ca826b9</a><o:p></o:p></span></pre>
<pre style="white-space:pre-wrap"><span style="color:black"><a href="https://github.com/facebookincubator/BOLT/commit/b40699ed90882465e49ec46e4d3ad28623e54c69">https://github.com/facebookincubator/BOLT/commit/b40699ed90882465e49ec46e4d3ad28623e54c69</a><o:p></o:p></span></pre>
<pre style="white-space:pre-wrap"><span style="color:black"><a href="https://github.com/facebookincubator/BOLT/commit/c21ee425083276a7af700e232df15e2a3d31dacb">https://github.com/facebookincubator/BOLT/commit/c21ee425083276a7af700e232df15e2a3d31dacb</a><o:p></o:p></span></pre>
<pre style="white-space:pre-wrap"><span style="color:black"><a href="https://github.com/facebookincubator/BOLT/commit/ebc06afcfc35fbb4360b3788adeb6228c27ad830">https://github.com/facebookincubator/BOLT/commit/ebc06afcfc35fbb4360b3788adeb6228c27ad830</a><o:p></o:p></span></pre>
<pre style="white-space:pre-wrap"><span style="color:black"><a href="https://github.com/facebookincubator/BOLT/commit/dca3003fce711daee387bbbf66bf4ecbe1b9ab9b">https://github.com/facebookincubator/BOLT/commit/dca3003fce711daee387bbbf66bf4ecbe1b9ab9b</a><o:p></o:p></span></pre>
<pre style="white-space:pre-wrap"><span style="color:black"><a href="https://github.com/facebookincubator/BOLT/commit/cd526a4550455b0f203771088cef6e512b0dd1a2">https://github.com/facebookincubator/BOLT/commit/cd526a4550455b0f203771088cef6e512b0dd1a2</a><o:p></o:p></span></pre>
<pre><span style="color:black">> 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.<o:p></o:p></span></pre>
<pre style="white-space:pre-wrap"><span style="color:black">> 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.<o:p></o:p></span></pre>
<p class="MsoNormal"><a href="https://github.com/facebookincubator/BOLT/commit/349c8abc696919e989418895f4ade24a4c910617">https://github.com/facebookincubator/BOLT/commit/349c8abc696919e989418895f4ade24a4c910617</a><o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Tue, Dec 21, 2021 at 4:22 AM Wang, Phoebe <<a href="mailto:phoebe.wang@intel.com">phoebe.wang@intel.com</a>> wrote:<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="color:#1F497D">Hi Amir,</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="color:#1F497D"> </span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="color:#1F497D">It makes sense to me. Thanks!</span><o:p></o:p></p>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span style="color:#1F497D"> </span><o:p></o:p></p>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><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<o:p></o:p></p>
</div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Hi Phoebe,<o:p></o:p></p>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">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.<o:p></o:p></p>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</body>
</html>