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

[Chandler Carruth via llvm-commits]

On Wed, Jun 8, 2016 at 6:49 PM Xinliang David Li via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

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

Yes, but I don't think this really adds anything but boilerplate *unless*
it was necessary for the new pass manager. Since it shouldn't be (because
we can instead use the pass managers LoopAnalysisManager directly), I don't
think having two classes is helpful here.


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

The DenseMap being split into the LoopAccessAnalysisResult object, separate
from the LoopAccessAnalysis function analysis pass.

If in the new pass manager this can be a *loop* analysis pass and the
LoopAccessInfo directly provided as the result, I think the legacy pass
manager won't really benefit from this separation.


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

The LoopAccessAnalysisResult added in this patch. The "analysis manager"
was a typo on my part, I meant to say "pass" referring to the
LoopAccessAnalysis which will eventually become a LoopAccessWrapperPass or
some such for the legacy pass 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
>>>>>
>>>>
>>>>
>>> _______________________________________________
> 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/3dfbfafe/attachment.html>