[PATCH] D31459: [Polly][NewPM] Port ScopDetection to the new PassManager

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: include/polly/ScopDetection.h:204-208
+  const DominatorTree &DT;
+  ScalarEvolution &SE;
+  LoopInfo &LI;
+  RegionInfo &RI;
+  AliasAnalysis &AA;
----------------
grosser wrote:
> philip.pfaffe wrote:
> > Meinersbur wrote:
> > > My personal opinion is to prefer pointers over references when the class is not copyable/value-like. References cannot be used in all contexts. The reason is consistency of how an object of a type is used independent from which function it is used in. It also avoids global rewriting such as this one.
> > > 
> > > However, this is my personal opinion and I see not everyone agrees with it.
> > I tentatively disagree. References are perfectly copyable. In turn, pointers convey one extra semantic, namely being nullable. This is of course nowhere being checked here.
> > Unless there's a reason to allow this extra state, I find references to be the better defalt.
> > 
> > It made sense to use pointers before, because they were lazily initialized. But now they aren't anymore.
> Even though I dislike global rewrites (as most of us), LLVM has a long tradition of not letting such dislikes prevent progress in terms of code quality and functionality. Hence, if there is an argument for a rewrite -- at best if it is mostly mechanical and just a single kind of change -- I am happy even with larger changes.
> 
> Now, I think Philip has a good point. If we can ensure the reference is never null, expressing this semantic by using references makes sense. Hence, I would agree with him and suggest to proceed with his choice assuming Michael is OK with this. If we do this, I would just suggest to mention the reason why this change was applied in the commit message.
> 
> Now, this is very subjective. Neither of the two choices is a lot better. One of the typical ways to get around bikesheding is to use 'grep' to get simple statistics what LLVM is doing. Sometimes this gives a clear picture, what we can follow. Otherwise, I suggest to agree on something and then follow this in Polly consistently.
I am OK with it.


================
Comment at: lib/Analysis/ScopDetection.cpp:272-321
+ScopDetection::ScopDetection(Function &F, const DominatorTree &DT,
+                             ScalarEvolution &SE, LoopInfo &LI, RegionInfo &RI,
+                             AliasAnalysis &AA)
+    : DT(DT), SE(SE), LI(LI), RI(RI), AA(AA) {
+
+  if (!PollyProcessUnprofitable && LI.empty())
+    return;
----------------
philip.pfaffe wrote:
> philip.pfaffe wrote:
> > Meinersbur wrote:
> > > I do not recommend having a lot of logic in a constructor:
> > > 
> > > - Makes composability more difficult. Assume we have to create this object in a different way. Then we'd have to clone the complete constructor although most members still have the 'obvious' initialization. Here: the analyses. This happened aready to the ScopStmt and MemoryAccess ctors.
> > > 
> > > - If an exception is raised, the destructors are called, on possibly uninitialized variables. This is problaby not what you expect.
> > > 
> > > - Calling an overridded virtual method calls the non-overridden method. This way it is possible to call an unimplemented method of an abstract class. Even if it is not an abstract class, if you are running an algorithm, you want the instantiated class' methods to be called, not one of the base classes. That is, better don't implement an algorithm in a constructor.
> > > 
> > > LLVM has exception switched off and ScopDetection is not derived from, so stricly speaking the 2. and 3. points do not appy. However, there is still the first and IMHO it is a good coding practice to only initialize members in constructors and nothing else. Expecially be careful when calling other members.
> > I will add a factory.
> Some bikeshedding on this:
> 
> - I disagree with your Composiability point. Right now this type serves a specific and narrow purpose. Widening this always requires some redesign.
> - Exception (un)safety is an issue, but only when there are managed resources. In the constructor, the object is fully initialized an in a defined state. Thus, if we stick to exception safe code (i.e., using RAII and/or exception safe resource management), we need not worry about exceptions.
> - This point I actually agree with fully. Right now there's no virtual dispatch here, but who knows if that might change in the future. So accepting this hazard right now might bite us in the future. 
> 
> In summary: I'd still like to do the full initialization in the constructor, because the full knowledge of the ValidRegions is the specific internal state that defines an object of this class (a factory would externalize this). To deal with the missing virtual dispatch in the constructor, I however propose moving a lot of the private interface of the ScopDetection class and its implementation out  out of the type. This would further satisfy your composability concerns. Thoughts?
  - Composability: Whether a class has one or more constructors does not depend on how specific the class'es purpose is. A new/different constructor might be required in different contexts it is used in or even for even narrower contexts. For instance, a ScopDetection might be instantiated only to verify that a given Region is a SCoP.

  - Exceptions: While this is true, it is easy to forget (especially by new contributors) and then add a constructor to manually free some resource. Again, this is theoretical (e.g. someone loading Polly in context where exceptions) since LLVM has exceptions disabled.

  - A factory is not the only possibility to avoid large constructors. You can also create a public method `findScops(Function &)` where the main work happens. `llvm::IDFCalculator` is an example of using this.

This is not a blocking issue for me, so if this is important for you, feel free to ignore my opinion. However, I think avoiding large constructors is preferable in general for the reasons already mentioned, even if these do not apply for a particulat case.


https://reviews.llvm.org/D31459