[llvm] r225131 - [PM] Split the AssumptionTracker immutable pass into two separate APIs:

Tue Jan 6 00:00:11 PST 2015

On Mon, Jan 5, 2015 at 11:55 AM, Hal Finkel <hfinkel at anl.gov> wrote:

> ----- Original Message -----
> > From: "Philip Reames" <listmail at philipreames.com>
> > To: "Chandler Carruth" <chandlerc at gmail.com>
> > Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> > Sent: Monday, January 5, 2015 1:22:28 PM
> > Subject: Re: [llvm] r225131 - [PM] Split the AssumptionTracker immutable
> pass into two separate APIs:
> >
> >
> > On 01/05/2015 11:12 AM, Chandler Carruth wrote:
> >
> >
> > Trying to respond to everything here. Let me know if my email client
> > has missed something...
> >
> >
> > On Mon, Jan 5, 2015 at 10:22 AM, Philip Reames <
> > listmail at philipreames.com > wrote:
> >
> > 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. 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.
> >
> >
> >
> > 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. 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.
> >
> > 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.
> >
> > 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.
> >
>
> 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.
>
> That having been said, if you have thoughts on how we can handle assumes
> more efficiently, I'd certainly like to hear them.
>
> >
> >
> > I wouldn't expect it to be "more expensive" in the sense that an
> > experiment would care about. I see two costs:
> >
> >
> > 1) compile time -- this should really be moderate at most. anything
> > more and we've got a bug that needs fixing 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.
> >
> > (Just to note: the piece of code that triggered this discussion has
> > not been in those profiles.)
> >
> >
> > 2) lost optimizations due to increased uses of common IR patterns
> >
> >
> > 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. I haven't run in to #2 in practice yet.
> >
> > Formatting changes (and arguably the rename involved in the API split
> > itself) should have been separated.
> >
> >
> > 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.
> > Honestly, this sounds like a feature request for clang-format to me.
> > :)
> >
> >
> >
> > 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?
> > 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.
> >
> >
> > 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. =/ Hm, I wonder if we could
> > make post commit review work via phabricator? Just a thought.
>
> I think we can: http://reviews.llvm.org/rL225131 -- I've never tried
> this, but if we were to add subscribers and comment perhaps that would work.
>
> (there must be some use to those 'Diffusion' e-mails I now get on every
> commit)
>

Not that I can figure. I turned them off.

>
>  -Hal
>
> >
> >
> >
> > 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.
> > :: nods ::
> >
> >
> > 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. I hadn't read this far when I wrote my comment
> > above. Nice to see we came to the same conclusion independently.
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150106/756e4909/attachment.html>