[llvm] [InstCombine] Do not request non-splat vector support in code reviews (NFC) (PR #90709)

[Nikita Popov via llvm-commits]

https://github.com/nikic created https://github.com/llvm/llvm-project/pull/90709

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 discourage 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 unnecessary complexity into InstCombine, or asking a first-time contributor to undo their changes, which is really not something I want to do.

Non-trivial 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 nowadays.

>From f3bba85dadde2d0d8db92da6582a713e62502ac9 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
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.