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

[Chandler Carruth]

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.


> 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

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.


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

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. =/


>
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150105/450b1e87/attachment.html>