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

[Xinliang David Li via llvm-commits]

On Wed, Jun 8, 2016 at 6:34 PM, Chandler Carruth <chandlerc at gmail.com>
wrote:

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

This change is NFC -- it simply wraps the the original Cache (DenseMap)
into a wrapper class with nicer interfaces, nothing more (and pretends to
be function level analysis result while in fact it is just a cache to the
loop level analysis result).

Can you be specific on what is 'function level result split out this way'?


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

I have hard time parsing this request too :)  Which result and analysis
manager?

thanks,

David



>
>
>>
>> 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/20160608/6df70c5d/attachment.html>