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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Thanks for getting this for started. The part of adding those passes to the new pass manager's pipeline must be done in RegisterPasses.cpp, but all passes must be registered to the new pass manager, otherwise it cannot use them.

Note that we have two correctness issues that are fragile with the pass managers:

1. Polly's codegen can change the IR of other SCoPs. Particularly it can make previously detected scops invalid. Thats why the sequence must be:

ScopDetection: A and B
ScopInfo on A, verify whether A is still a SCoP 
CodeGeneration of A
ScopInfo on B, verify whether B is still a SCoP
CodeGeneration of B

but not:

ScopDetection: A and B
ScopInfo on A
ScopInfo on B
CodeGeneration of A
CodeGeneration of B

CodeGeneration of A can invalidate B, resulting in miscompiles because the IR of B was changed and does not correspond to its ScopInfo anymore.

This is way we rely on the RegionPass to run all passes on region, the continue with the next region.

2. Polly does a bad job preserving analyses. It does not create new regions and loops in generated code. There is currently an ugly hack called NoopBarrier added to the pass pipeline that effectively throws away all analyses with the legacy pass manager. I get miscompiles when that barrier pass is removed. The new pass manager just caches all analyses.

Before these are not resolved, we cannot really use the new pass manager in production.



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


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


https://reviews.llvm.org/D31459