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

[Philip Reames]

Daniel,

I actively want to avoid coupling the EarlyCSE changes with the 
dominated use iterator work.  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?

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