[PATCH] D20560: [new PM] port LoopAccessAnalysis to new pass manager (part-1)

[Chandler Carruth via llvm-commits]

On Wed, Jun 8, 2016 at 6:31 PM Xinliang David Li <davidxl at google.com> wrote:

> Let me know if you want this change to be reverted. It does look like an
> incremental improvement so I guess not.
>

Not really sure.

I think you'll essentially end up reverting some of the changes here
because it doest (IMO) make sense to have the function-level result split
out this way. I'm not sure how much though.

Maybe it would make sense to juts fold the result stuff back into the
analysis manager but try to keep any thing in the LoopAccessInfo that
really belonged there?


>
> David
>
>
> On Wed, Jun 8, 2016 at 6:29 PM, Xinliang David Li <xinliangli at gmail.com>
> wrote:
>
>>
>>
>> On Wed, Jun 8, 2016 at 6:09 PM, Chandler Carruth via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> chandlerc added a comment.
>>>
>>> At a meta level, if I've started reviewing a patch, please wait until I
>>> have a chance to see your updates before submitting it. I'm sorry I didn't
>>> get to it in the early morning, but its still well under 24h turn around
>>> and I don't know that its reasonable to expect lower latency.
>>>
>>> That's not to say that I don't appreciate all of Adam and Davide's
>>> comments -- they're help reviewing is much appreciated -- but as it happens
>>> I don't actually think this is the right approach.
>>>
>>
>> Ah -- you had initial comments saying this makes sense -- thus I assumed
>> it is fine with lgtm.
>>
>> What you said below makes sense. I will rework on the patch to use
>> LoopAnalysisManager to see if works.
>>
>> thanks,
>>
>> David
>>
>>
>>>
>>> See below for more details.
>>>
>>>
>>> ================
>>> Comment at: llvm/trunk/include/llvm/Analysis/LoopAccessAnalysis.h:704-706
>>> @@ +703,5 @@
>>> +private:
>>> +  /// \brief LoopAccessInfo is created on demand. This map caches
>>> +  /// the computed results.
>>> +  DenseMap<Loop *, std::unique_ptr<LoopAccessInfo>> LoopAccessInfoMap;
>>> +
>>> ----------------
>>> This doesn't seem like the right design. It leads to the
>>> `getInfo().getInfo()` awkward pattern that IMO is only marginally less
>>> awkward spelled as `getResult().getInfo()`.
>>>
>>> Instead, I think this pretty clearly wants to be a Loop analysis in the
>>> new pass manager. I think that the caching and map should be the new pass
>>> manager's caching and map, rather than rolling our own in a function
>>> analysis manager.
>>>
>>> As a consequence, I don't think you need this result type at all. I
>>> think the LoopAccessInfo is already the correct result type. I think you'll
>>> need the DenseMap and logic around it in the legacy pass manager's function
>>> analysis, and you'll want a different query path to use the
>>> LoopAnalysisManager directly in the new pass manager.
>>>
>>> Does that make sense?
>>>
>>>
>>> Repository:
>>>   rL LLVM
>>>
>>> http://reviews.llvm.org/D20560
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160609/b42e2471/attachment.html>