This is an archive of the discontinued LLVM Phabricator instance.

[PM] Port Scalarizer to the new pass manager.
ClosedPublic

Authored by markus on Nov 19 2018, 3:48 AM.

Diff Detail

Event Timeline

markus created this revision.Nov 19 2018, 3:48 AM
bjope added a subscriber: bjope.Nov 19 2018, 3:53 AM
Ka-Ka added a subscriber: Ka-Ka.Nov 19 2018, 3:54 AM
markus added a subscriber: llvm-commits.

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.

markus updated this revision to Diff 174726.Nov 19 2018, 11:38 PM

Split into one wrapper class for the legacy PM and one wrapper class for the new PM.

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.
Why not converting them all?

markus updated this revision to Diff 174754.Nov 20 2018, 5:23 AM

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).

markus updated this revision to Diff 174762.Nov 20 2018, 6:20 AM

Moved ParallelLoopAccessMDKind into constructor.

fedor.sergeev accepted this revision.Nov 20 2018, 7:23 AM

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 dont have strong preference here, though his reasoning about shadowing InstVisitor's methods sorta makes sense.

This revision is now accepted and ready to land.Nov 20 2018, 7:23 AM

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.

../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);

You can use explicit qualification here:

bool Done = InstVisitor::visit(I);
markus updated this revision to Diff 174895.Nov 21 2018, 3:31 AM
fedor.sergeev accepted this revision.Nov 21 2018, 5:20 AM

Looks ok, feel free to push. Thanks!

This revision was automatically updated to reflect the committed changes.

I submitted this for markus since he doesn't have commit access yet.

I submitted this for markus since he doesn't have commit access yet.

Ah, sure, thanks!

bjope added inline comments.Nov 21 2018, 10:25 AM
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?
If Impl.visit returns false, it means that we didn't do any changes. And then I think it would be more appropriate to return PreservedAnalyses::all().

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.

return value for Scalarizer::run fixed in rL347432

return value for Scalarizer::run fixed in rL347432

Thanks! Looks nice!