With the (new) pass manager, passes can be either “required” or not required. For example, we don’t run optimization passes on functions marked optnone. By default passes are *not* required. To make a pass required, we add static bool isRequired() { return true; } like so:
class HelloWorldPass : public PassInfoMixin<HelloWorldPass> {
public:
PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
static bool isRequired() { return true; }
};
I’ve seen many cases where people forget to mark a pass as required. There shouldn’t be a default value for if a pass is required or not, every pass should explicitly make that choice.
This RFC proposes that we introduce MandatoryPassInfoMixin/OptionalPassInfoMixin which serve the purpose of marking if a pass is required or not, and then remove the default assumption that passes are not required.
To allow out-of-tree users to migrate their passes to this, we’ll keep the default assumption that passes are not required for now, and eventually remove that default later.
The patch seems quite disruptive, could a different structure make it less-so? Maybe if PassInfoMixin was for optional passes (so no new OptionalPassInfoMixin), and then there was MandatoryPassInfoMixin for mandatory passes?
I’m not very familiar with all the Concept/CRTP/etc tricks used in the mixin, and whether they require the optional vs mandatory classes to be separate in the class inheritance hierarchy.
PassInfoMixin still sounds like the “default” to me given it’s not qualified with “Mandatory” or “Optional”, and I’m explicitly trying to avoid a default. People will almost certainly still default to PassInfoMixin for all new passes without making a conscious decision as to whether the pass is required or not.
I’m hoping that out-of-tree passes are mostly optimization passes and people can s/PassInfoMixin/OptionalPassInfoMixin for the most part. You can see which passes are currently required by searching isRequired() since the default is false and passes typically don’t explicitly redundantly override it.
This is especially good to do now before we start porting a bunch of codegen pipeline passes where there are a lot more required lowering passes.
How many passes are supposed to be required vs not? Perhaps it can be addressed by some kind of end-to-end test with optnone/O0?
Also, looking at the changes, most “required” passes are just simple printers. Seems like a naming issue, where these passes are not really required/mandatory (for the code to compile and run correctly) but we just want them to still run with optnone (since they don’t/shouldn’t affect optimization). Perhaps some “PrinterPassMixin” (that’d also check that the pass doesn’t actually modify the IR) would make more sense (although not sure if there is a good way programmatically to enforce it).
I’m seeing 464 OptionalPassInfoMixin and 174 MandatoryPassInfoMixin after my change.
There are plenty of passes that don’t run in the optimization pipelines, and those are often the most problematic.
I’m happy to change the name to something like MustRunPassInfoMixin. I’m not sure there’s any reason to have a third PassInfoMixin like PrinterPassInfoMixin, that seems unnecessary when really the only thing is whether a pass should run on optnone functions.
Other potential names: AlwaysRunPassInfoMixin, RequiredPassInfoMixin (matching the current isRequired, although it is a little misleading like you say)
Required as the default is arguably better than the current state, but there’s out of tree work required with that as well.
Out-of-tree users will have to mark passes that previously didn’t have isRequired() with isRequired() { return false; }, which isn’t really any more work than s/PassInfoMixin/OptionalPassInfoMixin and changing the passes with isRequired() { return true; } to MandatoryPassInfoMixin.
I still think that forcing the user to choose mandatory/optional is better than defaulting to mandatory.
Of course the other alternative is the status quo, but I’ve seen multiple days lost to this from more than one person, and I think the migration pain is worth it. IMO it’s a relatively manageable migration.
How do these mixins interact with -{start,stop}-{before,after} in codegen pipeline? Currently, mandatory passes (e.g. requires) are not skipped.
Currently, passes like requires<some-analysis> could be filtered out but some passes may need them, regalloc passes are mandatory but we don’t want to skip them with optnone but -{start,stop}-{before,after}.
I’m fine with this change. Forcing an explicit decision here is good. Getting this wrong in either direction can be pretty annoying (that is, I don’t think making things required by default is a good choice either).
isRequired does affect if -opt-bisect-limit will skip a pass or not.
-O0 has its own short pipeline which doesn’t have any non-required passes. isRequired affects whether or not functions run on an optnone function, which the frontend will typically add under -O0.
Other potential names: AlwaysRunPassInfoMixin, RequiredPassInfoMixin (matching the current isRequired, although it is a little misleading like you say)
FWIW, I do think RequiredPassInfoMixin is better than MandatoryPassInfoMixin in order to stick with one terminology.