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

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added inline comments.


================
Comment at: include/polly/ScopDetection.h:204-208
+  const DominatorTree &DT;
+  ScalarEvolution &SE;
+  LoopInfo &LI;
+  RegionInfo &RI;
+  AliasAnalysis &AA;
----------------
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.


================
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;
----------------
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.


https://reviews.llvm.org/D31459