[RFC] Disallow unmaintained unused passes

LLVM Project
1

LLVM currently has a number of passes that are not used by the upstream project, i.e. are not part of any standard optimization pipeline.

Some of these passes are maintained and used by downstream users of LLVM. For example, various passes related to GC and deoptimization are not used by upstream LLVM, but are maintained by Azul. I believe having such passes upstream is both desirable and encouraged.

However, there are also passes that have no upstream use and also no upstream maintenance, beyond keeping up with API changes. To give an example, the LoopReroll pass is not part of the default optimization pipeline and has a long list of open miscompilation bug reports that nobody is working on.

When I proposed to remove it (⚙ D150684 [LoopReroll] Remove unused LoopReroll pass), I found out that there are actually is at least one downstream users of this pass, which has implemented their own fixes that were never upstreamed. I don’t think this is an acceptable state of affairs – if a pass is upstream, it also needs to be developed and maintained upstream. Otherwise, it should be kept downstream entirely.

Independently of this specific example, I would like to establish the following general policy:

All passes must satisfy at least one of the following:

  1. The pass is used by a standard optimization pipeline, or is a development/debugging pass. Standard optimization pipelines include those defined by PassBuilderPipelines (excluding those guarded by a default-disabled cl::opt option), passes injected by BackendUtil, as well as passes used by upstream backend targets.
  2. The pass has a documented code owner, who does at least fix reported miscompilations and crashes in a timely manner.

Otherwise, the pass may be removed.

Of course, it would be ideal to have a dedicated maintainer for all passes, whether they are used in the default pipeline or not, but for default passes we at least do fairly well with collective maintenance, even for passes that nobody actively works on otherwise.

I think removing unmaintained code should be a fairly uncontroversial position. I’m putting this up as an RFC mainly to make it clear that the only way to block a pass removal proposal is for somebody to actually step up, take personal responsibility for the pass, and become the new code owner.

9 Likes
2

Thanks for putting up this RFC! I think the outlined policy makes a lot of sense. Keeping those unmaintained/unused passes adds additional maintenance burden to the community and bringing the pass back is always possible as well once one of the outlined conditions gets satisfied.

It would probably be good to announce planned removals somewhere on Discord with a bit of a grace period to give people a chance to step up.

1 Like
3

+1 sounds reasonable to me.

I think announcements of removals should go to Discourse though, since that’s the official communications channel.

2 Likes
4

Yep I meant to say Discourse. The names are just too similar :stuck_out_tongue:

5

Yep, kill it. Whoever has downstream forks containing fixed versions of the pass gets to keep their downstream fork, except now it gets broken whenever upstream changes an API, which shifts the dev cost in favour of upstreaming fixes.

A related thing that I think is worth relaxing is whether functional changes without tests that reproduce in tree are accepted. I found a lot of breakage in sdag during the bringup for graphcore, but if I couldn’t also get a repro on x64 that was the end of my upstreaming path.

Sometimes the code in trunk looks broken, and someone has a downstream target that collided with it, but the no changes without tests enthusiasm means it only gets fixed downstream.

1 Like
6

I assume you are intentionally restricting the proposal to passes as opposed to general infrastructure (classes, functions, etc.)?

The latter might require more invasive changes that are more difficult to maintain downstream, see e.g [NFC][Bitcode] Remove unused param in DataLayoutCallbackTy by aeubanks · Pull Request #66705 · llvm/llvm-project · GitHub for a trivial example that was unused in upstream LLVM at the time (but now used by opt). I’m not sure though whether there is an official policy for these.

7

+1 to the original proposal. This seems like just documenting existing expectations.

8

+1 to the proposal, it makes sense.

9

+1

10

+1 for the proposal.