From fb77852c2263efcea341b589214f0843297bf3f2 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Wed, 1 May 2024 17:26:50 +0900 Subject: [PATCH] [InstCombine] Do not request non-splat vector support in code reviews (NFC) The InstCombine contributor guide already says: > Handle non-splat vector constants if doing so is free, but do > not add handling for them if it adds any additional complexity > to the code. I would like to strengthen this guideline to explicitly forbid asking contributors to implement non-splat support during code reviews. I've found that the outcome is pretty much always bad whenever this request is made. Most recent example is in #90402, which initially had a reasonable implementation of a fold without non-splat support. In response to reviewer feedback, it was adjusted to use a more complex implementation that supports non-splat vectors. Now I have the choice between accepting this unnecessary complexity into InstCombine, or asking a first-time contributor to undo their changes, which is really not something I want to do. Complex non-splat vector handling has done significant damage to the InstCombine code base in the past (mostly due to the work of a single contributor) and I am quite wary of repeating this mistake. --- llvm/docs/InstCombineContributorGuide.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/llvm/docs/InstCombineContributorGuide.md b/llvm/docs/InstCombineContributorGuide.md index 2416fd0920f62c..51d04dd074c56e 100644 --- a/llvm/docs/InstCombineContributorGuide.md +++ b/llvm/docs/InstCombineContributorGuide.md @@ -554,3 +554,11 @@ guidelines. use of ValueTracking queries. Whether this makes sense depends on the case, but it's usually a good idea to only handle the constant pattern first, and then generalize later if it seems useful. + +## Guidelines for reviewers + + * Do not ask contributors to implement non-splat vector support in code + reviews. If you think non-splat vector support for a fold is both + worthwhile and policy-compliant (that is, the handling would not result in + any appreciable increase in code complexity), implement it yourself in a + follow-up patch.