Details
Diff Detail
Event Timeline
To me it would be a cleaner solution if you could split legacy FunctionPass interface implementation into a separate wrapper pass (e.g. ScalarizerLegacyPass),
similar to how new-pm wrapper ScalarizerPass implements all the new-pm interfaces.
That would allow to separate actual transformation (Scalarizer) from pass-manager details (ScalarizerPass/ScalarizerLegacyPass).
Say, you would not need to perform legacy initializeScalarizerPass when initializing Scalarizer object for new-pm operations.
lib/Transforms/Scalar/Scalarizer.cpp | ||
---|---|---|
164 | Can this friend declaration be removed? |
Agree that splitting would give a cleaner solution. Will give that a go.
lib/Transforms/Scalar/Scalarizer.cpp | ||
---|---|---|
164 | If I do the splitting suggested above then yes, most likely it can. |
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...)
include/llvm/Transforms/Scalar/Scalarizer.h | ||
---|---|---|
10–15 | Make this a file-level doxygen comment? Or better yet, move it to be a class-level doxygen comment on the class below? | |
28 | 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. | |
lib/Transforms/Scalar/Scalarizer.cpp | ||
163 | Naming suggestion (but minor): you could call this ScalarizerVisitor and then name the variables below Visitor which seems to give a bit more information. | |
170–171 | 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 InstVisitors 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). | |
170–171 | 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. |
Overall it looks really good!
lib/Transforms/Scalar/Scalarizer.cpp | ||
---|---|---|
170–171 | Agreed, moving DT and metadata initialization to constructor seems to be a very logical step forward. | |
test/Transforms/Scalarizer/basic.ll | ||
2 | There are more scalarizer tests under test/Tranforms/Scalarizer. |
Hopefully this patch solves most of the comments I have received so far.
You may notice that some stuff about dominator trees have simply disappeared from the patch and this is because I previously, by accident, managed to include some downstream local changes that are note relevant for upstreaming.
Looks nearly ready to go.
lib/Transforms/Scalar/Scalarizer.cpp | ||
---|---|---|
305 | Would be nice to move this into the constructor (and thus pass ParallelLoopAccessMDKind parameter to that constructor and not to transform). |
LGTM, but feel free to address Chandlers' note if you dont have strong feelings against it.
lib/Transforms/Scalar/Scalarizer.cpp | ||
---|---|---|
169 | Chandler suggestion was to rename this method to 'visit'. |
I do not have a strong opinion about renaming 'transform' to 'visit' and would be happy to comply but simply doing a rename does not compile giving errors such as
../lib/Transforms/Scalar/Scalarizer.cpp:311:25: error: non-const lvalue reference to type 'llvm::Function' cannot bind to a value of unrelated type 'llvm::Instruction *' bool Done = visit(I); ^ ../lib/Transforms/Scalar/Scalarizer.cpp:302:41: note: passing argument to parameter 'F' here bool ScalarizerVisitor::visit(Function &F) { ^
so it would appear to require some additional changes to make it work.
llvm/trunk/lib/Transforms/Scalar/Scalarizer.cpp | ||
---|---|---|
822 ↗ | (On Diff #174912) | (sorry for post commit comments, but I ran into minor trouble when trying to merge this with our downstream branch) Not sure if it matters really, but why are we saying that we preserve "none" here? Isn't that what happens for the legacy version. When runOnFunction returns that nothing was modified it will implicitly also say that it preserve all analyses, even if getAnalysisUsage doesn't say so (or maybe I remember wrong about that). So there might be a slight difference about preserved analyses here, when the pass isn't doing any rewrites at all. |
@bjope:
Ugh... thanks for noting this, no idea how I managed to miss that during review.
Will patch it myself.
Make this a file-level doxygen comment? Or better yet, move it to be a class-level doxygen comment on the class below?