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

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Philip,

thanks for working on this. I just had a first view and I think the direction looks very good. There are still some style discussions open, but otherwise the patch looks great. I would like to get it in soon. The final thing missing is to actually be able to run and test this. What is needed to actually make this run and testable? I would really like to see a test case that verifies this is working. Also, could you add a comment in the commit message that explains how to run the viewers in the new pass manager?



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


https://reviews.llvm.org/D31459