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

[Xinliang David Li via llvm-commits]

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

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/ea631a89/attachment.html>