<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 01/05/2015 11:12 AM, Chandler
Carruth wrote:<br>
</div>
<blockquote
cite="mid:CAGCO0KjUN2pgY2KxiauascSrONXidD98ET7F12pmHy=2QKyCKA@mail.gmail.com"
type="cite">
<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 moz-do-not-send="true"
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>
</div>
</div>
</blockquote>
I don't have anything I can share unfortunately. You should be able
to write something reasonable by simply intermixing a long series of
assumes and other instructions (same basic block is fine) and
running it through inst combine. You might need to disable the
removal of redundant assumes to make writing test cases easier. <br>
<br>
The value tracking logic for assumes turns out to be really
expensive. We're visiting each dominating assume for each value we
look at - even when that assume has no relation to the value in
question. There's plenty of room to accelerate the current
implementation, but the structure seems less than ideal.<br>
<br>
<br>
<blockquote
cite="mid:CAGCO0KjUN2pgY2KxiauascSrONXidD98ET7F12pmHy=2QKyCKA@mail.gmail.com"
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"> <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>
</div>
</div>
</blockquote>
In practice, I'm seeing this happen. It hasn't risen up high enough
on my priority list to focus on, but I sometimes see as much as 70%
of total compile time spent processing assumes. <br>
<br>
(Just to note: the piece of code that triggered this discussion has
not been in those profiles.)<br>
<blockquote
cite="mid:CAGCO0KjUN2pgY2KxiauascSrONXidD98ET7F12pmHy=2QKyCKA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</div>
</blockquote>
I haven't run in to #2 in practice yet.<br>
<blockquote
cite="mid:CAGCO0KjUN2pgY2KxiauascSrONXidD98ET7F12pmHy=2QKyCKA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</div>
</div>
</blockquote>
When analysing the diffs, don't we have column information for what
changed? On the surface, it doesn't seem to hard to prevent the
formatter from changing white space outside of particular offsets.
<br>
<blockquote
cite="mid:CAGCO0KjUN2pgY2KxiauascSrONXidD98ET7F12pmHy=2QKyCKA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</div>
</blockquote>
Hm, I wonder if we could make post commit review work via
phabricator? Just a thought.<br>
<blockquote
cite="mid:CAGCO0KjUN2pgY2KxiauascSrONXidD98ET7F12pmHy=2QKyCKA@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
</blockquote>
I hadn't read this far when I wrote my comment above. Nice to see
we came to the same conclusion independently. <br>
<br>
</body>
</html>