<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:x="urn:schemas-microsoft-com:office:excel" 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:"\@SimSun";
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* 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";}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0in;
margin-right:0in;
margin-bottom:0in;
margin-left:.5in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;}
span.EmailStyle23
{mso-style-type:personal-compose;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
/* List Definitions */
@list l0
{mso-list-id:2095009764;
mso-list-type:hybrid;
mso-list-template-ids:1492444140 67698703 67698713 67698715 67698703 67698713 67698715 67698703 67698713 67698715;}
@list l0:level1
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level2
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level3
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l0:level4
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level5
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level6
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l0:level7
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level8
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level9
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
ol
{margin-bottom:0in;}
ul
{margin-bottom:0in;}
--></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">Hi Amir<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">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.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<ol style="margin-top:0in" start="1" type="1">
<li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo1"><a href="https://llvm.org/docs/CodingStandards.html#braced-initializer-lists">https://llvm.org/docs/CodingStandards.html#braced-initializer-lists</a> If you use a braced initializer
list when initializing a variable, use an equals before the open curly brace. e.g BinaryBasicBlock.h<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo1"><a href="https://llvm.org/docs/ProgrammersManual.html#fine-grained-debug-info-with-debug-type-and-the-debug-only-option">https://llvm.org/docs/ProgrammersManual.html#fine-grained-debug-info-with-debug-type-and-the-debug-only-option</a>
DEBUG_TYPE should not be target specific,<o:p></o:p></li></ol>
<p class="MsoListParagraph">e.g, the definition of DEBUG_TYPE should be same in X86MCPlusBuilder.cpp and AArch64MCPlusBuilder.cpp
<o:p></o:p></p>
<ol style="margin-top:0in" start="3" type="1">
<li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo1">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.<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo1">It would better if functions isADD, isAND, isCMP, isDEC, isINC, isSUB and isTEST can be generated with tablgen.<o:p></o:p></li></ol>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Best<o:p></o:p></p>
<p class="MsoNormal">Shengchen<o:p></o:p></p>
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> llvm-dev <llvm-dev-bounces@lists.llvm.org> <b>On Behalf Of
</b>Wang, Phoebe via llvm-dev<br>
<b>Sent:</b> Thursday, December 30, 2021 5:33 PM<br>
<b>To:</b> Amir Aupov <amir.aupov@gmail.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>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<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 <<a href="mailto:amir.aupov@gmail.com">amir.aupov@gmail.com</a>>
<br>
<b>Sent:</b> Thursday, December 30, 2021 4:32 PM<br>
<b>To:</b> Wang, Phoebe <<a href="mailto:phoebe.wang@intel.com">phoebe.wang@intel.com</a>><br>
<b>Cc:</b> <a href="mailto:llvm-dev@lists.llvm.org">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"><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-top:5.0pt;margin-right:0in;margin-bottom:5.0pt">
<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>