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

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added inline comments.


================
Comment at: include/polly/ScopInfo.h:2129
 
+  const StringRef getName() const { return name; }
+
----------------
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()?


================
Comment at: include/polly/ScopPass.h:36
+
+namespace llvm {
+using polly::Scop;
----------------
Meinersbur wrote:
> Is it necessary to have this in the llvm namespace? Normally we should stay in our Polly namespace.
> 
> If it is necessary, doesn't it mean this should be moved to LLVM (respectively that the new pass manager is not designed for extensibility)?
Stylistically I agree with your concern.

However, I'm specializing members here (the run() method and the Result member class). This must happen in an enclosing namespace of the template definition.


https://reviews.llvm.org/D32539