[PATCH] Extend EarlyCSE to handle basic cases from JumpThreading and CVP

[Daniel Berlin]

On Mon, May 18, 2015 at 4:30 PM, Philip Reames
<listmail at philipreames.com> wrote:
> Daniel,
>
> I actively want to avoid coupling the EarlyCSE changes with the dominated
> use iterator work.

Of course, that's why i simply attached it, outside the review, with
"FWIW".  I had it sitting around, so i attached it in case it was of
interest.

> I'm willing to help implement such a thing *separately*,
> but unless you strongly object, I would prefer to land the current version
> of the code since it's a strict improvement over what we have.


>
> Can you review the patch as is and either give me an explicit LGTM or a list
> of things you'd like me to address?

I was just about to review it.

>
> Philip
>
>
> On 05/14/2015 02:59 PM, Daniel Berlin wrote:
>>
>> -review because i don't want it to attach the patch i'm about to send.
>>
>> FWIW:
>> I've attached a working implementation of dominated_uses and
>> dominated_edge_uses
>> It could be a bit prettier (the ends can be made to not need
>> arguments), and maybe it should be a member function of something.
>>
>> I also lack the ability to reason about why filter_iterator must have
>> the using base::operator++ to get operator++(int) exposed from
>> iterator_facade, when it doesn't seem to be that way for any other
>> iterator_facade user (or maybe none of them use postfix increment, i
>> didn't look all that hard), *and* operator != simultaneously seems to
>> work happily from iterator_facade with nothing else.
>>
>>
>>
>>
>>
>> On Wed, May 13, 2015 at 5:30 PM, Philip Reames
>> <listmail at philipreames.com> wrote:
>>>
>>> I'll throw it in Local.h/cpp for the moment, but I may go back and write
>>> the
>>> dominated use iterator.  Doing that as a filter on an iterator range
>>> shouldn't be too challenging.  I definitely want to do that refactor as
>>> it's
>>> own change set though.
>>>
>>> I'll update the patch with the commonned functionality in a moment. What
>>> are
>>> you thoughts otherwise?
>>>
>>>
>>> On 05/13/2015 05:14 PM, Daniel Berlin wrote:
>>>>
>>>> This is a good question.
>>>>
>>>> I looked through, and i see that it is a common idiom, but often there
>>>> is a small amount of other code people are doing in the middle or as
>>>> part of replacement.  For example,
>>>> Transforms/ObjCARC/ObjCARCContract.cpp:552
>>>>
>>>> If it's not too much to ask, i actually think the best option may be
>>>> something like "a dominated use iterator".
>>>>
>>>> IE
>>>>
>>>> Instead of replaceAllDominatedUses(From, To, DomRoot)
>>>>
>>>> for (auto &Use : dominated_edge_uses(From, DomRoot))
>>>>     {
>>>>         // Do whatever extra stuff they  they want
>>>>        Use.set(To);
>>>>      }
>>>>
>>>>
>>>> and
>>>> instead of
>>>> replaceAllDominatedUses(From, To, From->getParent())
>>>>
>>>> for (auto &Use: dominated_uses(From))
>>>>     Use.set(To);
>>>>
>>>>
>>>> If that's too much, let's just put replaceAllDominatedUses in local.*
>>>> for
>>>> now.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Wed, May 13, 2015 at 4:13 PM, Philip Reames
>>>> <listmail at philipreames.com> wrote:
>>>>>
>>>>> ================
>>>>> Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:490
>>>>> @@ +489,3 @@
>>>>> +                  ToUpdate.push_back(IU);
>>>>> +            for (Instruction *UserI : ToUpdate) {
>>>>> +              DEBUG(dbgs() << "EarlyCSE CVP: Replace dominated use of
>>>>> '"
>>>>> ----------------
>>>>> dberlin wrote:
>>>>>>
>>>>>> Can you common this part with GVN's replaceAllDominatedUsesWith?
>>>>>>
>>>>>> (IE just move that function somewhere common and use it?)
>>>>>
>>>>> Absolutely.  Didn't know that existed.  Would it make sense to put that
>>>>> directly on Value?  Or would you rather see it as a helper function in
>>>>> Local.h or someplace similar?
>>>>>
>>>>> http://reviews.llvm.org/D9763
>>>>>
>>>>> EMAIL PREFERENCES
>>>>>     http://reviews.llvm.org/settings/panel/emailpreferences/
>>>>>
>>>>>
>