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

[Philip Reames]

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 <mailto: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 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.
>
>
>     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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150105/c2cc7e7c/attachment.html>