<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 5, 2015 at 11:55 AM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">----- Original Message -----<br>
> From: "Philip Reames" <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>><br>
> To: "Chandler Carruth" <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>><br>
> Cc: "Commit Messages and Patches for LLVM" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>><br>
</span><span class="">> Sent: Monday, January 5, 2015 1:22:28 PM<br>
> Subject: Re: [llvm] r225131 - [PM] Split the AssumptionTracker immutable pass into two separate APIs:<br>
><br>
><br>
</span><div><div class="h5">> On 01/05/2015 11:12 AM, Chandler Carruth wrote:<br>
><br>
><br>
> Trying to respond to everything here. Let me know if my email client<br>
> has missed something...<br>
><br>
><br>
> On Mon, Jan 5, 2015 at 10:22 AM, Philip Reames <<br>
> <a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a> > wrote:<br>
><br>
> Secondarily, I'm not sure that having more assumes is very common.<br>
> Introducing an assume intrinsic is a reasonable high-cost thing to<br>
> the rest of the optimizer, and so I was being relatively<br>
> conservative in the expected number of them. But this is something<br>
> that should be tuned based on data if the initial guessed value<br>
> isn't really suitable. One data point: I'm using them fairly heavily<br>
> at the moment. I don't plan on continuing to do so for the current<br>
> use - which is mostly to get a really nasty exception scheme to<br>
> optimize reasonably - but imagine I'll run across something else I<br>
> need them for in the future.<br>
><br>
><br>
><br>
> Interesting. Do you have benchmarks today? I'm curious if this is a<br>
> performance problem, and don't have any realistically heavy usage to<br>
> compare with. I don't have anything I can share unfortunately. You<br>
> should be able to write something reasonable by simply intermixing a<br>
> long series of assumes and other instructions (same basic block is<br>
> fine) and running it through inst combine. You might need to disable<br>
> the removal of redundant assumes to make writing test cases easier.<br>
><br>
> The value tracking logic for assumes turns out to be really<br>
> expensive. We're visiting each dominating assume for each value we<br>
> look at - even when that assume has no relation to the value in<br>
> question. There's plenty of room to accelerate the current<br>
> implementation, but the structure seems less than ideal.<br>
><br>
> On point worth thinking about: If we want assumes to be our mechanism<br>
> for rapid experimentation w.r.t. optimizer hints, adding assumes<br>
> can't be too much more expensive than an attribute or metadata.<br>
> Otherwise, it makes quick experimentation much harder.<br>
><br>
<br>
</div></div>I disagree. Using assumes to experiment with what optimizer hints might be useful is different from saying you're going to deploy those hints in production using assumes. If you need a lot of them, for production use, you'll need a different mechanism.<br>
<br>
That having been said, if you have thoughts on how we can handle assumes more efficiently, I'd certainly like to hear them.<br>
<div><div class="h5"><br>
><br>
><br>
> I wouldn't expect it to be "more expensive" in the sense that an<br>
> experiment would care about. I see two costs:<br>
><br>
><br>
> 1) compile time -- this should really be moderate at most. anything<br>
> more and we've got a bug that needs fixing In practice, I'm seeing<br>
> this happen. It hasn't risen up high enough on my priority list to<br>
> focus on, but I sometimes see as much as 70% of total compile time<br>
> spent processing assumes.<br>
><br>
> (Just to note: the piece of code that triggered this discussion has<br>
> not been in those profiles.)<br>
><br>
><br>
> 2) lost optimizations due to increased uses of common IR patterns<br>
><br>
><br>
> I think #2 is the inherent limitation of the current design of<br>
> assumptions -- it was an explicit tradeoff in the design that made<br>
> sense in part due to folks expecting them to not be pervasive. That<br>
> said, I suspect this wouldn't really matter for experimentation<br>
> either... But if folks end up hitting a wall here, that would be<br>
> interesting to find out. I haven't run in to #2 in practice yet.<br>
><br>
> Formatting changes (and arguably the rename involved in the API split<br>
> itself) should have been separated.<br>
><br>
><br>
> It's nearly impossible to separate these kinds of formatting changes.<br>
> I didn't reformat anything by hand here, I simply asked clang-format<br>
> to re-format the lines I had touched in the patch. In several cases<br>
> this was necessary as the correct formatting was different after the<br>
> s/AssumptionTracker/AssumptionCache/ change. For better or worse,<br>
> clang-format will also fix other formatting issues (such as X* vs X<br>
> *) in the same "unwrapped line". It doesn't really add any noise to<br>
> the blame though as I was going to touch this code either way.<br>
> Honestly, this sounds like a feature request for clang-format to me.<br>
> :)<br>
><br>
><br>
><br>
> We actually wanted this, and desperately. We've not figured out how<br>
> to get it. Let the clang-format folks know if you have any ideas?<br>
> When analysing the diffs, don't we have column information for what<br>
> changed? On the surface, it doesn't seem to hard to prevent the<br>
> formatter from changing white space outside of particular offsets.<br>
><br>
><br>
> Anyways, instead, we have a code review tool that does a much better<br>
> job of highlighting the non-whitespace change within a patch. Sadly,<br>
> email doesn't work so well for this. =/ Hm, I wonder if we could<br>
> make post commit review work via phabricator? Just a thought.<br>
<br>
</div></div>I think we can: <a href="http://reviews.llvm.org/rL225131" target="_blank">http://reviews.llvm.org/rL225131</a> -- I've never tried this, but if we were to add subscribers and comment perhaps that would work.<br>
<br>
(there must be some use to those 'Diffusion' e-mails I now get on every commit)<br></blockquote><div><br>Not that I can figure. I turned them off.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-Hal<br>
</font></span><span class="im HOEnZb"><br>
><br>
><br>
><br>
> The only reason I commented here is that the first time I saw this<br>
> line (scanning quickly), I didn't see the restructuring change and<br>
> did see the formatting. It wasn't until I stared at it for a second<br>
> that I saw the other.<br>
> :: nods ::<br>
><br>
><br>
> Maybe some day phabricator will have a nice post-commit review flow<br>
> that makes it much easier to see these kinds of changes and<br>
> understand them. I hadn't read this far when I wrote my comment<br>
> above. Nice to see we came to the same conclusion independently.<br>
><br>
><br>
</span><div class="HOEnZb"><div class="h5">> _______________________________________________<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>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<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>