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

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added a comment.

This patch is currently blocked by an ugly bug in RegionInfo, which I'll have to sort out first before I can move this further. Apologies!



================
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:
> 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?


https://reviews.llvm.org/D31459