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

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev added a comment.

Overall it looks really good!



================
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:
> 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.
Agreed, moving DT and metadata initialization to constructor seems to be a very logical step forward.


================
Comment at: test/Transforms/Scalarizer/basic.ll:2
 ; RUN: opt %s -scalarizer -scalarize-load-store -dce -S | FileCheck %s
+; RUN: opt %s -passes='function(scalarizer,dce)' -scalarize-load-store -S | FileCheck %s
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
----------------
There are more scalarizer tests under test/Tranforms/Scalarizer.
Why not converting them all?


https://reviews.llvm.org/D54695