<div dir="ltr"><div>Dahao,</div>I think the basic algorithm should be captured somewhere (preferably in patch's summary)<div>It would be useful to know the algorithm quickly rather than going through source code.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 19, 2016 at 1:19 PM, Chandler Carruth via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">chandlerc added a comment.<br>
<br>
This is really close. Some minor nit picks and a few more interesting comments below.<br>
<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/<wbr>LoopUtils.h:472-474<br>
<span class="">+/// If SafetyInfo is nullptr, we are checking for sinking instructions from<br>
+/// preheader to loop body (no speculation).<br>
+/// If SafetyInfo is not nullptr, we are checking for hoisting/sinking<br>
</span>----------------<br>
<span class="">chandlerc wrote:<br>
> Here and elsewhere in comments, I would just say "is null" rather than "is nullptr".<br>
</span>You changed one 'nullptr' to 'null' but missed the other.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>LoopSink.cpp:11-12<br>
+// This pass does the inverse transformation of what LICM does.<br>
+// It traverses all of the instructions in the loop's preheader and sink<br>
+// them/ to the loop body where frequency is lower than the loop's preheader.<br>
<span class="">+// This pass is a reverse-transformation of LICM. It differs from the Sink<br>
</span>----------------<br>
"and sink them/ to" -> "and sinks them to"<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>LoopSink.cpp:101<br>
</span>+///<br>
+/// /p SortedLoopBBs is used to help find the optimal sinking locations.<br>
+/// It stores a list of BBs that is:<br>
----------------<br>
"/p" -> "\p"<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>LoopSink.cpp:159<br>
</span>+ BlockFrequencyInfo &BFI) {<br>
+ if (I.getNumUses() > MaxNumberOfUsesForSinking)<br>
+ return false;<br>
----------------<br>
The number of user *instructions* isn't really the right thing to apply the threshold to as that doesn't directly change the cost.<br>
<br>
The idea is that we need the size of `BBsToSinkInto` to be a small constant in order for the search for the coldest dominating set to be "just" linear in the number of blocks in the loop.<br>
<br>
So while a threshold of "40" may make sense for number of user instructions, I suspect the threshold should be much smaller when applied to the size of `BBsToSinkInto`.<br>
<br>
I also think you should add two comments about this. One, you should comment to the `findBBsToSinkInto` function clarifying the algorithmic complexity (That it O(N * M) or O(M^2) where N is SortedLoopBBs.size() and M is BBsToSinkInto.size()), and you should mention where you check this threshold that the reason is because we're going to call `findBBsToSinkInto` which would go quadratic if we didn't set a cap.<br>
<br>
The reason for all of this is that I'm worried some future maintainer will come along and not really understand how risk it is to adjust these thresholds so I think it is important to document the implications.<br>
<br>
I still think we will long-term need a better algorithmic approach here as I suspect we'll find annoying cases where the threshold blocks an important optimization (such as when there are way too many initial BBsToInsertInto but there are a small number of common dominating blocks). But I understand this is one of the really hard problems (its the same as optimal PHI placement and a bunch of other ones), and I don't want to hold up your patch on a comprehensive approach here.<br>
<br>
<br>
On an unrelated note, you should also document that this threshold has a secondary function: it places an upper bound on how much code growth we may trigger here. I'd document this in particular as that seems somewhat accidental and I suspect we may long-term want a better threshold for that. I would in fact encourage you to leave a FIXME to adjust this for min_size and opt_size.<br>
<br>
<br>
<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>LoopSink.cpp:178-179<br>
+ findBBsToSinkInto(L, BBs, SortedLoopBBs, DT, BFI);<br>
+ if (BBsToSinkInto.size() == 0)<br>
<span class="">+ return false;<br>
+<br>
----------------<br>
</span>We generally prefer calling `.empty()` to testing `.size()` against zero.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>LoopSink.cpp:184<br>
+ BBsToSinkInto.end());<br>
+ std::stable_sort(<wbr>SortedBBsToSinkInto.begin(), SortedBBsToSinkInto.end(),<br>
<span class="">+ [&](BasicBlock *A, BasicBlock *B) {<br>
</span>----------------<br>
Here you don't need stable sort since this is a total ordering. You should just use std::sort and mention that this is a known total ordering in a comment. You could do that in an overall comment htat explains what you're doing here:<br>
<br>
// Copy the final BBs into a vector and sort them using the total ordering<br>
// of the loop block numbers as iterating the set doesn't give a useful<br>
// order. No need to stable sort as the block numbers are a total ordering.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>LoopSink.cpp:194<br>
<span class="">+<br>
+ for (++BI; BI != BBsToSinkInto.end(); ++BI) {<br>
+ BasicBlock *N = *BI;<br>
</span>----------------<br>
You didn't actually switch to the sorted list here.<br>
<br>
Also, you can just use a range based for loop here.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>LoopSink.cpp:218<br>
<span class="">+/// sum frequency of inserted copy is smaller than preheader's frequency.<br>
</span>+bool sinkLoop(Loop &L, AAResults &AA, LoopInfo &LI, DominatorTree &DT,<br>
+ BlockFrequencyInfo &BFI, ScalarEvolution *SE) {<br>
----------------<br>
This routine doesn't sink the loop, so a name `sinkLoop` seems confusing. Maybe `<wbr>sinkLoopInvariantInstructions`<wbr>?<br>
<br>
Also, I think this should be a static function.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>LoopSink.cpp:241-246<br>
<span class="">+ SmallVector<BasicBlock *, 10> SortedLoopBBs;<br>
</span>+ DenseMap<BasicBlock *, int> LoopBlockNumber;<br>
<span class="">+ int i = 0;<br>
</span>+ for (BasicBlock *B : L.blocks())<br>
+ if (BFI.getBlockFreq(B) <= BFI.getBlockFreq(L.<wbr>getLoopPreheader())) {<br>
+ SortedLoopBBs.push_back(B);<br>
----------------<br>
It wasn't obvious to me reading this that `SortedLoopBBs` only contained the oop BBs that are less hot than the preheader. I think it might be nice to clue the reader in that this isn't *all* the loop BBs. Maybe `SortedColdLoopBBs`? Or just `ColdLoopBBs`? If you make this change, I'd keep the name consistent throughout of course.<br>
<br>
Also, you use `<=` here, but `<` everywhere else I see, any particular reason to include BBs in this list with the same frequency?<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<a href="https://reviews.llvm.org/D22778" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D22778</a><br>
<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><i style="font-size:12.8px">Disclaimer: Views, concerns, thoughts, questions, ideas expressed in this mail are of my own and my employer has no take in it. </i><br></div><div>Thank You.<br>Madhur D. Amilkanthwar<br><br></div></div></div>
</div>