<div dir="ltr"><div class="gmail_extra"><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></blockquote><div><br></div><div>I'm hoping to avoid needing post-dominance information as we currently don't compute it in the standard optimization pipeline. But again, this is (IMO) really for a subsequent patch.</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: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></blockquote><div><br></div><div>Sorry, I meant just the SmallVector so that we clear rather than reallocate.</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: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></blockquote><div><br></div><div>If you have an if-then-else diamond, you cannot propagate up into the if block because the then block doesn't post-dominate the if block?</div>
<div><br></div><div>I still see the issue of the continue when you have already found an equivalence class for the basic block... However due to the symmetry, I can't yet think of a case where this is necessary.</div>
<div><br></div><div>It's fine to leave it here for now though. I can mull on it later.</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>
> ================<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>
<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>
<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>
<span class="HOEnZb"><font color="#888888"><br>
<br>
Diego.<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>