[PATCH] D32539: [Polly][NewPM][WIP] Add a ScopPassManager

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Philip,

this already looks very good. I am pleased to see the overall structure written out. I understand that there are still pieces missing, but as this is nicely independent of everything else I believe we can gradually build this up in LLVM. Hence, I believe committing this already soon makes sense such that people can play and experiment with it.

Now, what would be really useful is:

1. The ability to actually run (and test) individual passes with 'opt'

2. The ability to run polly in -O3

I assume 2) is only possible after your patch has been accepted by Chandler. As mentioned earlier, Polly-specific #ifdefs might be a good idea to early on establish a full flow.

I am not clear what is missing for 1).

Best,
Tobias



================
Comment at: include/polly/ScopInfo.h:2129
 
+  const StringRef getName() const { return name; }
+
----------------
philip.pfaffe wrote:
> Meinersbur wrote:
> > There is already `getNameStr`. Why the need for another one?
> The PassManager expects IR-Units to provide a getName() function. Constructing and returning a std::string (instead of StringRef) from this seems wasteful.
> 
> Assuming the the information used in getNameStr() to build the name doesn't change over the lifetime of the Scop, does it make sense to drop getNameStr(), store the name in the Scop itself and return that from getName()?
If you believe getNameStr() could be implemented more efficiently, then we can do this. However, I would assume getName[Str]() is only called when actually dumping a scop. So eagerly constructing the name string might even have a negative effect performance wise.

I personally do not have a strong opinion if we have getNameStr or getName or if we create the name eagerly or not, until this becomes significant in any profile I see. So if you prefer one over the other I am OK with this.

However, from my perspective I would ignore likely minimal performance effects for now and just include what is needed for this patch without introducing redundancies.


================
Comment at: include/polly/ScopInfo.h:1530
   Region &R;
+  /// The name of the SCoP (identical to the regions name)
+  std::string name;
----------------
Newline before comment??


================
Comment at: include/polly/ScopPass.h:9-10
 //===----------------------------------------------------------------------===//
-//
-// This file defines the ScopPass class.  ScopPasses are just RegionPasses,
-// except they operate on Polly IR (Scop and ScopStmt) built by ScopInfo Pass.
-// Because they operate on Polly IR, not the LLVM IR, ScopPasses are not allowed
-// to modify the LLVM IR. Due to this limitation, the ScopPass class takes
-// care of declaring that no LLVM passes are invalidated.
+// // This file defines the ScopPass class.  ScopPasses are just RegionPasses,
+// // except they operate on Polly IR (Scop and ScopStmt) built by ScopInfo
+// Pass. Because they operate on Polly IR, not the LLVM IR, ScopPasses are not
----------------
Meinersbur wrote:
> double `//`?
This just seems a reformatting of the original comment, without any actual changes. Can you just leave the original format and keep this part unchanged?


================
Comment at: lib/Analysis/ScopPass.cpp:63
+  // aren't affected
+  // FIXME is this correct wrt. the current pass behaviours?
+  PA.preserveSet<AllAnalysesOn<Scop>>();
----------------
All passes that just work on 'Scop' should not affect other scops. However, the code generation passes have indeed the potential to affect other scops.

An option that is potentially the most savest option is add more logic into the scoppassmanager. This manager could construct one scopinfo at a time, run all scop passes (except code generation), and then does code generation itself. It then constructs the next scopingo, runs all passes, and does code generation.

This however is possibly unnecessary complicated. So my feeling is that we should ignore this for now, assume everything is safe, get test cases, and build up the flow. After we got something working, we can get test coverage and failing cases and then look into how to handle them best. My feeling is that the general structure should not change, but might just become slightly more complex. As this is independent to everything else in Polly and unlikely to regress anything, I believe we can develop this in-tree.


================
Comment at: lib/Analysis/ScopPass.cpp:92
+
+  bool allPreserverd = PA.allAnalysesInSetPreserved<AllAnalysesOn<Scop>>();
+
----------------
allPreserved


https://reviews.llvm.org/D32539