[DeveloperPolicy] Add guidelines for adding/enabling passes #158591
Conversation
| a flag that is disabled by default. | ||
| 2. Enable the pass by default. Separating this step allows easily disabling the | ||
| pass if issues are encountered, without having to revert the entire | ||
| implementation. |
There was a problem hiding this comment.
This seems implicit, but may be worth making it explicit a bit more: this strategy seems to rely on the first implementation to be "conservative": that is the "basic version" of the pass is about "doing less" in term of usefulness, but "not miscompiling anything" right?
There was a problem hiding this comment.
I've added an extra sentence "The initial version should focus on handling simple cases correctly and efficiently" here. Is that clearer? Happy to take suggestions on how to improve the wording here.
| least one maintainer. | ||
| * **Usefulness:** There should be evidence that the pass improves performance | ||
| (or whatever metric it optimizes for) on real-world workloads. Improvements | ||
| seen only on synthetic benchmarks may be insufficient. |
There was a problem hiding this comment.
Maybe we should consider distinguishing -O2 and -O3? We already run a lot of passes that do nothing for most functions. For -O2 (the average RelWithDebInfo build) we might want to focus on transforms that have an impact for a broader range of programs and not just a few special workloads.
There was a problem hiding this comment.
Hm, I guess aspirationally the answer to this is "yes", but I think in practice it is "no". We don't really have a strong distinction between the -O2 and -O3 pipelines, beyond using different inlining/unrolling thresholds. The differences that do exist between the pipelines are not good ones (for example, there is no good reason why ArgumentPromotion should be an O3 pass, it is very much generally useful).
There was a problem hiding this comment.
I know, but maybe we want to change this at some point? Loop interchange, for example, is likely beneficial for certain programs (probably numerics), but for the average integer-heavy C++ program, it probably has no impact (except for increasing compile times). Perhaps we can start by asking authors for an assessment of the range of programs that will benefit from the transformation if they believe that it should be enabled at -O2?
| still complies with current quality standards. Fuzzing with disabled | ||
| profitability checks may help gain additional confidence in the | ||
| implementation. | ||
|
|
There was a problem hiding this comment.
Fuzzing with disabled profitability checks may help gain additional confidence in the implementation.
Yeah, I agree, and for the record, that's exactly what I did with loop-interchange:
- I first added a9f8143
- Then I ran fuzzers for many days and found many many bugs, i.e. all the bugs on this first page: https://github.com/llvm/llvm-project/issues?q=is%3Aissue%20author%3Asjoerdmeijer, which includes one DA issue and 1 or two in interchange.
Anyway, that is slightly irrelevant for this policy, but just wanted to get this out of the way from previous discussions.
Running fuzzers is slightly less straightforward than it sounds. A good documentation contribution would be to add a howto for this. I am not suggesting it should be here in this section, but I can look where that belongs and prepare a draft how to do this.
There was a problem hiding this comment.
Yeah, I'm aware that you did this. I thought it was a sufficiently good idea to include it as a general recommendation :)
Running fuzzers is slightly less straightforward than it sounds. A good documentation contribution would be to add a howto for this. I am not suggesting it should be here in this section, but I can look where that belongs and prepare a draft how to do this.
Sounds good to me.
|
Looks generally OK to me. |
| where the evaluation of what "large" means is up to reviewer discretion, and | ||
| may differ based on the value the pass provides. In any case, it is expected | ||
| that a concerted effort has been made to mitigate the compile-time impact, | ||
| both for the average case, and for pathological cases. |
There was a problem hiding this comment.
Do you want to suggest to use your LLVM compile-time tracker for this?
There was a problem hiding this comment.
Yes, a reference to the latest compile-time tracker results should be required. How much impact on compile-time is acceptable? (For loop interchange, we heard 0.19% is "too much"). While we are at it, let's also quantify how much is too much.
Edit: I see that it is left to the reviewer's discretion, but isn't that open-ended?
There was a problem hiding this comment.
I think it's not really possible to name a specific number here, because the compile-time impact is a tradeoff against the value the pass provides. A pass that provides a performance benefit on a wide range of code will have a larger compile-time budget than a pass that only benefits rare cases.
llvm/docs/DeveloperPolicy.rst
Outdated
| enabled, it becomes easier to identify the specific change that has caused a | ||
| regression in correctness, optimization quality or compile-time. | ||
|
|
||
| When enabling a pass, regardless of whether it is old or new, certain |
There was a problem hiding this comment.
Might be worth just dropping the old/new distinction, as the steps apply to any pass that needs enabling regardless of the distinction?
There was a problem hiding this comment.
I've dropped the distinction in this sentence, but I do think separating old/new is important for the correctness aspect. The bar for establishing confidence in the correctness of a pass implemented 15 years ago is different than from one that we reviewed just yesterday.
This documents two things:
RFC: https://discourse.llvm.org/t/rfc-guidelines-for-adding-enabling-new-passes/88290