<div dir="ltr"><div class="gmail_extra">First off, sorry this is now in two threads....</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 25, 2014 at 5:57 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.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="">On Mar 25, 2014, at 2:06 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>

> 2) Outside of the loop structure detection and management, how important are the RPO-traversal bits? Was it just convenient or really important to propagate weights in this way? This could be the real limitation of using LoopInfo -- it doesn't really preserve the RPO structures the way your code does.<br>

<br>
</div>The RPO-traversal is important for distributing mass (you need everything to<br>
arrive at a node before you know how much to distribute to a successor).<br></blockquote><div><br></div><div>Yea, that makes sense.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
Nevertheless, I’m also starting to feel skeptical about rolling my own,<br>
particularly because it’s complicated code that’s somewhat orthogonal to what<br>
I’m doing, so it really needs its own set of tests.<br>
<br>
I’d like to commit it as is, and then try to reuse LoopInfo before turning the<br>
new block frequency on.<br></blockquote><div><br></div><div>And this makes a great deal of sense (similarly in the other thread). I like the general plan here.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
> 3) I'm increasingly in favor of just using power-of-two loop scales. I actually can't come up with any use cases where distinguishing between 3 and 4 as the "likely" loop trip count would matter. The only case I can see would be that rounding 3 down to 2 could have a bad effect in 3-iteration-heavy code such as graphics code, but it seems simpler to fix that directly by rounding 3 explicitly up to 4…<br>

<br>
Andy was worried about numbers up to 8.  It should be straightforward to<br>
hardcode the boundaries for the scales 3, 5, 6, and 7, and then use power-of-<br>
two scales for the rest.<br></blockquote><div><br></div><div>Ok. I still don't see the importance of 5, 6, and 7... Maybe we could get away with just special casing 3 if these special cases add much complexity? Maybe they don't add much complexity.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The code will get even harder to look at, though.  To prevent the precision<br>
issues I was hitting originally, I’ll have to do some floating point-like<br>
stuff.  I’m fine either way, but it will be even harder for someone else to<br>
come and figure out what’s going on.<br></blockquote><div><br></div><div>Could it be encapsulated in an even narrower utility which is simpler to understand? I don't fully understand the impact of this, and I'm hesitant to suggest writing the code both ways just to stare at it and throw one away. And I can see how the PositiveFloat abstraction is in some senses an easier thing to build and work with in the abstract.</div>
<div><br></div><div>I was hoping that the way this would play out would be to compute the loop scales, and then just fall back on saturating integer math.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="">> 4) I'm having trouble with the mixture of terminology between mass, weight, and frequency. Do you have a mental model for the terminology you can add to the documentation? (Or did I miss it?)<br>
<br>
</div>I do have a mental model; some of it was in the docs, but not all.  I’ll fix<br>
that.<br>
<div class=""><br>
> I'm also still concerned about exposing both mass and frequency in the public API. What is the plan there?<br>
<br>
</div>BlockMass was only exposed so that I could test BlockMass*BranchProbability.  I<br>
think the right solution is to move that into BranchProbability*uint64_t and test<br>
it there.<br>
<br>
I.e., I don’t think it should be exposed.</blockquote></div><br>Ok. I think having the terminology clarified will also help me understand the code.</div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div>
<div class="gmail_extra">The only thing that I think really needs to be sorted out (other than the stuff you've clarified you're planning to do already) prior to commit is the positive float issue. I'm just a bit hesitant to put that into Support only to rip it back out. ;] But maybe that doesn't matter too much.</div>
</div>