optnone/skipping passes in the new pass manager

Looking through some of the remaining test failures under the new pass manager, I’ve narrowed down one of the failures in GWPAsan(!) to the fact that the new pass manager doesn’t properly skip passes like the old pass manager. For example, when a function is marked optnone, or when using https://llvm.org/docs/OptBisect.html.

Lots of passes (e.g. SROA) will do the following under the legacy pass manager:

bool runOnFunction(Function &F) override {
if (skipFunction(F))
return false;

// do pass
}

What’s the right way to proceed with this? There are 50-100 calls to skipFunction() in legacy passes. This doesn’t even account for other types of IR units, like skipModule(Module&).

I suppose it’s possible to manually go in and add in the same check in the new passes, but that seems tedious (and how do you test that at scale? clearly there aren’t many tests for it right now since check-llvm passes under the new pass manager). An alternative of skipping passes at the pass manager level would require marking each pass as necessary/optional (I think…).

When the optnone design was being discussed, Chandler specifically rejected having the pass manager involved in the decision (which was the original proposed design). Assuming he still feels the same way now, if the existing skipFunction calls aren’t being executed under the new pass manager, then each pass that has that call will need to be modified accordingly (added to the NPM path or moved to some common point). It would be best if the skipFunction calls were handled consistently in all passes so that it would become part of the normal pass boilerplate.

I suspect any skipFunction or opt-bisection tests have been written to force the old pass manager, which is why defaulting to the new pass manager doesn’t fail anywhere.

–paulr

re: Paul: Sounds about right. (+Chandler just in case he wants to weigh in)

When the optnone design was being discussed, Chandler specifically rejected having the pass manager involved in the decision (which was the original proposed design). Assuming he still feels the same way now, if the existing skipFunction calls aren’t being executed under the new pass manager, then each pass that has that call will need to be modified accordingly (added to the NPM path or moved to some common point). It would be best if the skipFunction calls were handled consistently in all passes so that it would become part of the normal pass boilerplate.

Makes sense, thanks for the background. I’ll see if there’s a clean way to make this functionality easy to use in the NPM.

I suspect any skipFunction or opt-bisection tests have been written to force the old pass manager, which is why defaulting to the new pass manager doesn’t fail anywhere.

I took a closer look at some optnone tests and they use opt instead of clang. opt still defaults to the legacy pass manager regardless of clang’s default.

Maybe you could change the default PM in opt and see what fails?

–paulr

Hmm it looks like getting NPM to work with opt is non-trivial. Only a small portion of the opt functionality works with NPM :frowning:

Hmm it looks like getting NPM to work with opt is non-trivial. Only a small
portion of the opt functionality works with NPM :frowning:

I am very much looking forward to use the NPM by default but this sounds like a problem we need to fix first :(.

You happen to have some "list" of things that are missing?

Hmm it looks like getting NPM to work with opt is non-trivial. Only a small
portion of the opt functionality works with NPM :(

I am very much looking forward to use the NPM by default but this sounds like a problem we need to fix first :(.

I’ll look into this some more.

You happen to have some “list” of things that are missing?

Not really, my process is fairly straightforward, set the ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER CMake flag to TRUE, run check-all, and look at new failures. So far these two issues (skipping passes and opt) are the biggest issues I’ve stumbled across (and mostly by luck, like I said there were a couple failures in GwpAsan that unearthed this, at least for me). I’m still relatively new so I might be missing some existing list of NPM issues that somebody else more experienced has come up with?

I think adding a before-pass callback checking optnone to PassInstrumentation of NPM should suffice for the `skipFunction` alone. However, it is closely related to opt-bisect which makes it more complicated.

This was the previous discussion.
http://lists.llvm.org/pipermail/llvm-dev/2018-September/126477.html

Thanks for the link to the previous discussion, that’s super helpful! I’ll have to read it over more closely again.

That does remind me of another item on the NPM TODO list: codegen is still using the legacy pass manager. I don’t think it’s a blocker as long as things like opt-bisect work with it (as mentioned in the previous discussion).