<div dir="ltr"><div class="gmail_extra">Trying to respond to everything here. Let me know if my email client has missed something...</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 5, 2015 at 10:22 AM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Secondarily, I'm not sure that having more assumes is
very common. Introducing an assume intrinsic is a
reasonable high-cost thing to the rest of the optimizer,
and so I was being relatively conservative in the expected
number of them. But this is something that should be tuned
based on data if the initial guessed value isn't really
suitable.</div>
</div>
</div>
</div>
</blockquote></span>
One data point: I'm using them fairly heavily at the moment. I
don't plan on continuing to do so for the current use - which is
mostly to get a really nasty exception scheme to optimize reasonably
- but imagine I'll run across something else I need them for in the
future. <br></blockquote><div><br></div><div>Interesting. Do you have benchmarks today? I'm curious if this is a performance problem, and don't have any realistically heavy usage to compare with.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On point worth thinking about: If we want assumes to be our
mechanism for rapid experimentation w.r.t. optimizer hints, adding
assumes can't be too much more expensive than an attribute or
metadata. Otherwise, it makes quick experimentation much harder.<br></blockquote><div><br></div><div>I wouldn't expect it to be "more expensive" in the sense that an experiment would care about. I see two costs:</div><div><br></div><div>1) compile time -- this should really be moderate at most. anything more and we've got a bug that needs fixing</div><div><br></div><div>2) lost optimizations due to increased uses of common IR patterns</div><div><br></div><div>I think #2 is the inherent limitation of the current design of assumptions -- it was an explicit tradeoff in the design that made sense in part due to folks expecting them to not be pervasive. That said, I suspect this wouldn't really matter for experimentation either... But if folks end up hitting a wall here, that would be interesting to find out.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="">
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="overflow:hidden">
<div>
<div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
</blockquote>
</div>
</div>
Formatting changes (and arguably the rename involved in
the API split itself) should have been separated.</div>
</blockquote>
<div><br>
</div>
<div>It's nearly impossible to separate these kinds of
formatting changes. I didn't reformat anything by hand
here, I simply asked clang-format to re-format the lines I
had touched in the patch. In several cases this was
necessary as the correct formatting was different after
the s/AssumptionTracker/AssumptionCache/ change. For
better or worse, clang-format will also fix other
formatting issues (such as X* vs X *) in the same
"unwrapped line". It doesn't really add any noise to the
blame though as I was going to touch this code either way.</div>
</div>
</div>
</div>
</blockquote></div></div>
Honestly, this sounds like a feature request for clang-format to
me. :)<br></blockquote><div><br></div><div>We actually wanted this, and desperately. We've not figured out how to get it. Let the clang-format folks know if you have any ideas?</div><div><br></div><div>Anyways, instead, we have a code review tool that does a much better job of highlighting the non-whitespace change within a patch. Sadly, email doesn't work so well for this. =/</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The only reason I commented here is that the first time I saw this
line (scanning quickly), I didn't see the restructuring change and
did see the formatting. It wasn't until I stared at it for a second
that I saw the other. </blockquote></div><br>:: nods ::</div><div class="gmail_extra"><br></div><div class="gmail_extra">Maybe some day phabricator will have a nice post-commit review flow that makes it much easier to see these kinds of changes and understand them.</div></div>