[PATCH] D54695: [PM] Port Scalarizer to the new pass manager.
Fedor Sergeev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 20 03:55:28 PST 2018
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
More information about the llvm-commits
mailing list