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

[Nikita Popov via llvm-commits]

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

>From fb77852c2263efcea341b589214f0843297bf3f2 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 1/2] [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.

>From 1a04be33125470cea5bfc3bed61c504c447d27d8 Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Fri, 3 May 2024 12:05:30 +0900
Subject: [PATCH 2/2] Add "new"

---
 llvm/docs/InstCombineContributorGuide.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/llvm/docs/InstCombineContributorGuide.md b/llvm/docs/InstCombineContributorGuide.md
index 51d04dd074c56e..ce5f958058c550 100644
--- a/llvm/docs/InstCombineContributorGuide.md
+++ b/llvm/docs/InstCombineContributorGuide.md
@@ -557,7 +557,7 @@ guidelines.
 
 ## Guidelines for reviewers
 
- * Do not ask contributors to implement non-splat vector support in code
+ * Do not ask new 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