Skip to content

Commit

Permalink
[InstCombine] Do not request non-splat vector support in code reviews…
Browse files Browse the repository at this point in the history
… (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 llvm#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.
  • Loading branch information
nikic committed May 3, 2024
1 parent d2af1ea commit fb77852
Showing 1 changed file with 8 additions and 0 deletions.
8 changes: 8 additions & 0 deletions llvm/docs/InstCombineContributorGuide.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

0 comments on commit fb77852

Please sign in to comment.