<div dir="ltr">Arrrg, mashed send too soon... here is the rest of the replies...<div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 10, 2014 at 11:59 AM, Diego Novillo <span dir="ltr"><<a href="mailto:dnovillo@google.com" target="_blank">dnovillo@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Thu, Jan 9, 2014 at 7:35 AM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>> wrote:<br>
<br>
> It was not clear to me at first that this is an inversion flag. I'd like a better name, but I don't have any particular ideas. I wonder if it would be more clear by passing in the domtree pointer to check?<br>
<br>
</div>Done. I changed the parameter to pass a DominatorTreeBase pointer. The<br>
caller passes DT->DT or PDT->DT accordingly.<br>
<div class="im"><br>
<br>
> ================<br>
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:563-565<br>
> @@ +562,5 @@<br>
> + BasicBlock *BB2 = *I;<br>
> + bool IsDomParent =<br>
> + (CheckDT) ? DT->dominates(BB2, BB1) : PDT->dominates(BB2, BB1);<br>
> + bool IsInSameLoop = LI->getLoopFor(BB1) == LI->getLoopFor(BB2);<br>
> + if (BB1 != BB2 && VisitedBlocks.insert(BB2) && IsDomParent &&<br>
> ----------------<br>
> I suspect there is a more direct way to compute this, but I don't want to hold up the review thinking about it -- maybe a FIXME?<br>
><br>
> Specifically, I think that we might be able to phrase this purely in terms of LoopInfo and DomTree.<br>
<br>
</div>Sorry, I'm not following. This is expressed in terms of loop info and<br>
dominance. What do you want to change?<br>
<div class="im"><br>
> ================<br>
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:578-581<br>
> @@ +577,6 @@<br>
> + // members of its equivalence set.<br>
> + uint32_t &bb1_weight = BlockWeights[BB1];<br>
> + uint32_t &bb2_weight = BlockWeights[BB2];<br>
> + if (bb2_weight > bb1_weight)<br>
> + bb1_weight = bb2_weight;<br>
> + }<br>
> ----------------<br>
> bb1_weight = std::max(bb1_weight, bb2_weight);<br>
<br>
</div>D'oh! Done.<br>
<div class="im"><br>
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:620<br>
> @@ +619,3 @@<br>
> + // class by making BB2's equivalence class be BB1.<br>
> + SmallVector<BasicBlock *, 8> DominatedBBs;<br>
> + DT->getDescendants(BB1, DominatedBBs);<br>
> ----------------<br>
> You should probably put this outside the loop.<br>
<br>
</div>That would be wrong. BB1 changes with every iteration of the loop.<br>
<div class="im"><br>
><br>
> ================<br>
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:624-625<br>
> @@ +623,4 @@<br>
> +<br>
> + // Repeat the same logic for all the blocks post-dominated by BB1.<br>
> + // We are looking for every basic block BB2 such that:<br>
> + //<br>
> ----------------<br>
> Why do we need to do this?<br>
><br>
> If we visit every block as BB1, and then visit every block BB1 dominates as BB2, we will have considered all pairs of blocks where A dominates B, and tested for B post-dominating A. We don't need to consider pairs of blocks where B post-dominates A because we will reject the ones where A does not dominate B.<br>
<br>
</div>Note that we do not necessarily visit every block as BB1. We only<br>
visit the blocks for which we have equivalence class information. This<br>
means that initially, only those blocks with samples in them have been<br>
assigned into an equivalence class.<br>
<br>
So, if in a if-then-else diamond, you only have samples inside the<br>
'then' basic block, you need to look up from the 'then' to be able to<br>
propagate into the 'if'. Using just dominance properties for 'then'<br>
will not allow you to propagate its weight.<br>
<div class="im"><br>
<br>
><br>
> ================<br>
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:578<br>
> @@ +577,3 @@<br>
> + // members of its equivalence set.<br>
> + uint32_t &bb1_weight = BlockWeights[BB1];<br>
> + uint32_t &bb2_weight = BlockWeights[BB2];<br>
> ----------------<br>
> Chandler Carruth wrote:<br>
>> bb1_weight = std::max(bb1_weight, bb2_weight);<br>
> Naming convention here.<br>
<br>
</div>OK, though I'm pretty confused. Your line_iterator patch uses this<br>
convention. And, in fact, I've seen _ used as separator in several<br>
places. Do we have different conventions for different parts of the<br>
compiler? The coding guidelines don't seem to say anything about<br>
having different conventions.<br></blockquote><div><br></div><div>The only exception is when the interface is mimicing a standard library interface. Then we try to follow the naming conventions of the standard library. So there are several iterators or similar standard-library-like interfaces that use a different naming convention. For general LLVM code, that doesn't apply.</div>
<div><br></div><div>Sorry for that confusion.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
><br>
> ================<br>
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:940<br>
> @@ +939,3 @@<br>
> +///<br>
> +/// - If all the edges are known except one, and the weight of the<br>
> +/// block is already known, the weight of the unknown edge will<br>
> ----------------<br>
> Incoming edges or outgoing edges? Or both?<br>
<br>
</div>Incoming or outgoing. Clarified.<br>
<div class="im"><br>
> ================<br>
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:937-938<br>
> @@ +936,4 @@<br>
> +///<br>
> +/// - If B has a single predecessor/successor, then the weight<br>
> +/// of that edge is the weight of the block.<br>
> +///<br>
> ----------------<br>
> How do you handle contradictions?<br>
<br>
</div>From user annotations? I would give a warning, if I could. Right now,<br>
it's a silent override.<br></blockquote><div><br></div><div>No, contradiction in the samples. You can have random samples set up situations that are inexplicable. Such as zero count errors on loop headers.</div><div>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> ================<br>
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:951-953<br>
> @@ +950,5 @@<br>
> +///<br>
> +/// Since this propagation is not guaranteed to finalize for every CFG, we<br>
> +/// only allow it to proceed for a limited number of iterations (controlled<br>
> +/// by -sample-profile-max-propagate-iterations).<br>
> +///<br>
> ----------------<br>
> This is a really frustrating property. Can it be easily avoided?<br>
<br>
</div>Not without changing the algorithm itself.<br>
<div class="im"><br>
> The first of the above steps is obviously deterministic.<br>
><br>
> For the second and third, I feel like the following is guaranteed to make forward progress and terminate:<br>
> 0) Add all blocks to a list to process.<br>
> 1) Take the first block out of the list<br>
> 2) if we can add a weight to an edge connected to this block (including a self referential edge) do so and add the other block the edge connects to to the list (if not already in the list)<br>
> 3) repeat #2 until we cannot add a weight to any edge connected to this block<br>
> 4) if the list is not empty, go to #1<br>
> We only add a block to the list if we add a weight to an edge, so we can at most visit V+E blocks.<br>
<br>
</div>Yes, I thought about a similar scheme (in fact, mimicking standard<br>
value propagation). But this changes the algorithm. Which is something<br>
I was trying to avoid at first.<br></blockquote><div><br></div><div>OK, definitely leave a FIXME about this. LLVM works very hard to avoid this style of "iterate enough times" algorithms, and it feels like the closed form algorithm accomplishes the same fundamental goal so I don't expect this to be a big source of different results.</div>
</div></div></div>