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

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added a comment.

> 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).

2. is tied very closely to PassBuilders ability to accept injected plugin passes, yes.

On the other hand, while it would be technically possible to forward declare the polly passes in PassBuilder (and the PassRegistry.def), that would pose a severe violation of layering, because we'd need to make lib/Passes depend on polly, both logically as well as in a build-system sense. This dependency doesn't currently exist: LLVM_LINK_POLLY_INTO_TOOLS (which would be the flag we'd #ifdef on), only affects tools. Forward declaring Polly in the PassBuilder would however pull polly dependencies into the PassBuilder.

The good news is I've talked to Chandler about this some more, and there will be progress on the Plugins patch soon!



================
Comment at: include/polly/ScopInfo.h:2129
 
+  const StringRef getName() const { return name; }
+
----------------
grosser wrote:
> 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.
I'm completely in favor of lazily building the name.

That would mean initializing the name member on the first call of getName(). I didn't dare to rename getNameStr() (or, in other words, return std::string from getName()), because I can't guarantee that this function won't be called in a context where a StringRef is expected, thus creating a dangling reference to a termporary.




================
Comment at: lib/Analysis/ScopPass.cpp:63
+  // aren't affected
+  // FIXME is this correct wrt. the current pass behaviours?
+  PA.preserveSet<AllAnalysesOn<Scop>>();
----------------
grosser wrote:
> 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.
There is actually a rather clean way to implement this, and it's very similar to what the LoopPassManager does today. We can pass an Updater along the Scop pipeline down into CodeGeneration. Using that we can propagate ScopInfo invalidations into the ScopPassManager which can then skip now-invalid Scops. This works under two conditions:

  # Invalidation occurs only for Scops that haven't been transformed yet. If I understand the current implementation correctly, that's condition holds.
  # CodeGeneration actually knows which potential Scops it breaks. From the previous discussions I'm under the impression  that there are only a handful of ways it does that, so it sounds like it's easily checkable. Worst case I assume the Updater would need to verify all remaining Scops.

I expect this to be a an uninvasive change. Most of the work would need to go into CodeGeneration in figuring out which Scops to skip.



https://reviews.llvm.org/D32539