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

[Hal Finkel]

----- 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)

 -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