[PATCH] D54695: [PM] Port Scalarizer to the new pass manager.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Bunch of somewhat minor API cleanups that seem reasonable to do when touching these specific bits of the code...

(Please, feel free to continue / finish the review Fedor, just wanted to throw these out while I was looking at the code...)



================
Comment at: include/llvm/Transforms/Scalar/Scalarizer.h:9-14
+//
+// This pass converts vector operations into scalar operations, in order
+// to expose optimization opportunities on the individual scalar operations.
+// It is mainly intended for targets that do not have vector units, but it
+// may also be useful for revectorizing code to different vector widths.
+//
----------------
Make this a file-level doxygen comment? Or better yet, move it to be a class-level doxygen comment on the class below?


================
Comment at: include/llvm/Transforms/Scalar/Scalarizer.h:27
+  PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
+};
+}
----------------
I've also tried to move the `create...` functions to build the legacy pass into these headers when introducing them as I think that makes things much cleaner.


================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:164
 
-class Scalarizer : public FunctionPass,
-                   public InstVisitor<Scalarizer, bool> {
+class Scalarizer : public InstVisitor<Scalarizer, bool> {
 public:
----------------
Naming suggestion (but minor): you could call this `ScalarizerVisitor` and then name the variables below `Visitor` which seems to give a bit more information.


================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:167
 
-  void getAnalysisUsage(AnalysisUsage& AU) const override {
-    AU.addRequired<DominatorTreeWrapperPass>();
-    AU.addPreserved<DominatorTreeWrapperPass>();
-  }
+  bool doTransform(Function &F, DominatorTree *DT, unsigned ParallelLoopAccessMDKind);
 
----------------
While here, I would suggest removing the `do` from the name as it doesn't add any information. `transform` is already a great verb for this function.

Actually, I might suggest naming this `visit` to explicitly shadow the `InstVisitor`s `visit` methods as it would be an error for a user to accidentally call one of those on this class (there is clearly very precise logic for this one that needs to be used).


================
Comment at: lib/Transforms/Scalar/Scalarizer.cpp:167
 
-  void getAnalysisUsage(AnalysisUsage& AU) const override {
-    AU.addRequired<DominatorTreeWrapperPass>();
-    AU.addPreserved<DominatorTreeWrapperPass>();
-  }
+  bool doTransform(Function &F, DominatorTree *DT, unsigned ParallelLoopAccessMDKind);
 
----------------
chandlerc wrote:
> While here, I would suggest removing the `do` from the name as it doesn't add any information. `transform` is already a great verb for this function.
> 
> Actually, I might suggest naming this `visit` to explicitly shadow the `InstVisitor`s `visit` methods as it would be an error for a user to accidentally call one of those on this class (there is clearly very precise logic for this one that needs to be used).
Since this is never called with a null dominator tree, how about accepting it by reference?

Personally, I'd move everything but the function to the constructor so that the main interface to this visitor mirrors that of the underlying `InstVisitor` with just top-level `visit` methods.


https://reviews.llvm.org/D54695