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

Xinliang David Li via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 8 18:29:39 PDT 2016


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


More information about the llvm-commits mailing list