Page MenuHomePhabricator

[new PM] port LoopAccessAnalysis to new pass manager (part-1)
ClosedPublic

Authored by davidxl on May 23 2016, 10:15 PM.

Details

Summary

part-1 is a NFC refactoring patch to enable porting. Changes include

  1. renamed LoopAccessAnalysis.h to be LoopAccessInfo.h
  2. split out LoopAccessAnalysis class into new file LoopAccessAnalysis.h
  3. introduced LoopAccessFuncInfo class to represent function level analysis result of LoopAccess infos
  4. machinery to support lazy computation of LoopAccessInfo

Diff Detail

Event Timeline

davidxl updated this revision to Diff 58194.May 23 2016, 10:15 PM
davidxl retitled this revision from to [new PM] port LoopAccessAnalysis to new pass manager (part-1).
davidxl updated this object.
davidxl added a reviewer: bogner.
davidxl added subscribers: llvm-commits, chandlerc.
davide added a subscriber: davide.May 23 2016, 10:24 PM

The number of different changes here make it really hard to review.

Can you instead actually send out the individual changes?

I also don't see any benefit to renaming the file. I think the "Info" suffix, as common as it is in LLVM, is probably a mistake. I'm not sure its worth going and fixing everything to use a different suffix, but I like the current file name better than the one you propose here. (Just my opinion, if others have a different view from working more closely with the loop access stuff or vectorization stuff, totally fine with that...)

The renaming also makes the diff hard to read which I don't like either. I
will revert the renaming part.

thanks,

David

davidxl updated this revision to Diff 59835.Jun 6 2016, 11:38 PM

Addressed review comments: do not rename existing header file and minimizes diffs.

PTAL

Addressed review comments: do not rename existing header file and minimizes diffs.

PTAL

I'm not sure what is expected to be changing here? There are four things in your patch description, and my suggestion was to split them apart. It looks like this isn't split apart, just adjusted?

I'm also not sure why you're adding the file to LoopAccessInfoAnalysis.h?

Maybe it would help to provide an updated over-all summary? I feel like I may just not understand what is going on here.

New summary of the change:

  1. split the legacy LoopAccess Analysis pass out of the header file LoopAccessAnalysis.h
  2. introduced a new class LoopAccessFuncInfo that represents the function level analysis results that is cacheble.
  3. move the interfaces that access the loop level LoopAccessInfo from legacy pass class to the new LoopAccessFuncInfo class
  4. change clients to use the new interfaces

Does it make sense?

Thanks, this definitely helps.

New summary of the change:

  1. split the legacy LoopAccess Analysis pass out of the header file LoopAccessAnalysis.h

Any particular reason to split these apart? It would seem simpler to just do the refactoring in place in the header file...

  1. introduced a new class LoopAccessFuncInfo that represents the function level analysis results that is cacheble.

Makes sense. I'd probably call it LoopAccessInfo to fit with the common pattern, but I'm happy if there are reasons to name this differently.

  1. move the interfaces that access the loop level LoopAccessInfo from legacy pass class to the new LoopAccessFuncInfo class
  2. change clients to use the new interfaces

    Does it make sense?

Yep. See one question on the code I have initially below...

include/llvm/Analysis/LoopAccessInfoAnalysis.h
36–45 ↗(On Diff #59835)

Why this pattern?

Specifically, why deviate from the pattern that other analyses that have been ported use where you just construct the result class with pointers to the various other analysis results they reference?

Introducing a complete abstract interface seems like a much heavier weight abstraction; unless there is a specific problem solved, I'd rather the lighter weight abstraction.

davidxl updated this revision to Diff 59941.Jun 7 2016, 1:24 PM

Addressed Chandler's feedback.

anemet added a subscriber: anemet.Jun 7 2016, 3:53 PM
anemet added inline comments.
lib/Transforms/Scalar/LoopDistribute.cpp
611

getInfo().getInfo(...), really?

What is the difference between LAFI and LAA conceptually?

Without knowing much about the new PM, I see that you split out the cache from LAA (which was pretty much all it contained).

Should this perhaps be:

LAA->getCache().getInfo(..)

or getLoopInfo(..)

?

davide added inline comments.Jun 7 2016, 9:26 PM
include/llvm/Analysis/LoopAccessAnalysis.h
684

I'm not entirely sure what's the preferred LLVM style here, but I seldom (if ever) have seen variables ending with _ used like this.
I might miss something (and certainly I didn't compile), but, won't
this.AAR = AAR et similia work?

701

Move this outside of the function, and make it doxygen, i.e.
\brief Invalidate the cache when this pass is freed

706

This comment is too generic to be somehow useful. I think it should be expanded a bit, while you're here.

709

Why AAR and not AA as it was before? Also more consistent with how other variables are named.

lib/Analysis/LoopAccessAnalysis.cpp
1966

As you're passing getAnalysis everywhere, can't you also inline
TLIP ? &TLIP->getTLI() : nullptr instead of assigning TLI and passing the variable?

lib/Transforms/Scalar/LoopLoadElimination.cpp
555

I agree with Adam here, the getInfo().getInfo() is just very weird

davidxl updated this revision to Diff 59997.Jun 7 2016, 10:00 PM

Addressed Adam and Davide's review comments.

anemet accepted this revision.Jun 8 2016, 3:24 AM
anemet added a reviewer: anemet.

I am fine with this with comments below addressed. Thanks!

include/llvm/Analysis/LoopAccessAnalysis.h
677

This needs a class comment. Also what do you think about naming this Result as well, i.e. LoopAccessAnalysisResult?

679–680

You don't need to call the default ctor for LAIM like this. Also you may want to use in-class data member initializers for the nullptrs and then remove the ctor altogether.

This revision is now accepted and ready to land.Jun 8 2016, 3:24 AM
davidxl updated this revision to Diff 60059.Jun 8 2016, 10:06 AM
davidxl edited edge metadata.

Addressed Adam's feedback.

LGTM with the change below.

include/llvm/Analysis/LoopAccessAnalysis.h
748

s/LAFI/LAAR

davide accepted this revision.Jun 8 2016, 11:52 AM
davide added a reviewer: davide.

LGTM once you address Adam's last comment. Thanks for your patience.

This revision was automatically updated to reflect the committed changes.

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.

See below for more details.

llvm/trunk/include/llvm/Analysis/LoopAccessAnalysis.h
704–706 ↗(On Diff #60092)

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?

anemet added inline comments.Jun 9 2016, 2:37 AM
llvm/trunk/include/llvm/Analysis/LoopAccessAnalysis.h
704–706 ↗(On Diff #60092)

Just for the record, this had to be a function pass because the loop vectorizer is a function pass with the legacy PM.

It would be great to turn this into a loop pass with the new PM.