As some people have requested more clarity on what our criteria for adding or enabling new optimization passes are, I’ve put up some draft wording here:
Please provide feedback either here or on the PR.
As I wrote on the merge request, I am happy with this, and in particular the part that talks about implementing a basic version first, which then gets enabled by default, and further incrementally developed in-tree. I hope this avoids the mess that we are currently in with all the loop optimisations that are off by default. I will admit that I contributed to this, but have changed my opinion on this and default enablement should be a first objective and not some after-thought, otherwise it is more likely not going to happen.
The patch starts with “The guidelines here are primarily targeted at the enablement of new major passes in the target-independent optimization pipeline”. This is true, but as it gives advice/direction on how and when new passes should be added to the repo, “enablement” and “adding” (or ‘upstreaming’) become somewhat intertwined, and I wonder if that catches all cases we care about?
I think of a pass like ImplicitNullChecks - it’s not a “backend-specific pass”, perhaps doesn’t seem appropriate for turning on by default in a general purpose pipeline, but it seems good that it exists in LLVM for people to use if appropriate for their workloads.
I’m not sure if this is a case of needing a small rewording like “Small additions, or backend-specific passes, require a lesser degree of care” => “Small additions, backend-specific passes, or workload specific passes not intended for the default target-independent optimization pipeline require a lesser degree of care”. Or perhaps we do have a different understanding?
I think the key question is: do we not think there are non-target-specific passes that make sense to upstream into LLVM that are not expecting to be added to the default optimisation pipeline?
Thanks for bringing that up. It’s always tricky to find the right wording that catches all the nuance. I added this parenthetical to the text:
(This does not apply to passes that are only relevant for specific uses of LLVM, such as GC support passes.)
Having passes that are not relevant for Clang (or anything else in the monorepo) but are useful to multiple external projects are useful to have upstream, as this provides a common place for those projects to collaborate, instead of implementing N diverging downstream implementations of the same thing.
The strong push for “aim for default enablement ASAP” in this text is because we have a lot of prior experience that basically says: Either a new pass gets enabled right away, or it will take a very long time to enable it, if it happens at all. Enabling passes long after they have been implemented causes a lot of additional work and frustration for everyone involved.
1 Like
Thanks for the clarification, it makes total sense to me now it has that additional text. I strongly agree that we do want to continue encouraging the upstreaming of these kind of passes.
Just being pedantic because ImplicitNullChecks was used as the example… ImplicitNullChecks was developed “on by default” from the beginning; it was just enabled for a different (downstream) frontend. The text coverage and confidence built through ongoing testing was critical as we expanded it, and improved what it could handle.
I think this is a valuable point (not necessarily for the policy document per se). Even for code whose end user isn’t clang, having it developed using the same methodology will be beneficial in the long run. Having an established user, with ongoing testing, and the ability to develop incrementally helps whichever frontend you’re targeting.
And maybe this goes without saying, but enabling code for a second frontend is much easier once it’s been enabled by default for a while for one. We did find a bunch of bugs in e.g. instcombine (subtle differences in typical IR), but these were way easier to get fixed.
I think these are good guidelines. Do you mind if I copy some of the ideas for GCC when I write up an RFC there? GCC has had less a problem of disabled upstream passes than LLVM has had in the past; though the maintaince side of things has been an issue with GCC passes too. GCC has an issue of many passes that do a similar thing that should be combined (which I started to cleanup).
The PR at [DeveloperPolicy] Add guidelines for adding/enabling passes by nikic · Pull Request #158591 · llvm/llvm-project · GitHub has a couple of approvals, so I’m planning to merge this soon, if there is no further feedback.
Sorry, I missed this question. You are of course welcome to copy/use this on the GCC side as well.