Skip to content

Commit fb77852

Browse files
committed
[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 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.
1 parent d2af1ea commit fb77852

File tree

1 file changed

+8
-0
lines changed

1 file changed

+8
-0
lines changed

llvm/docs/InstCombineContributorGuide.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -554,3 +554,11 @@ guidelines.
554554
use of ValueTracking queries. Whether this makes sense depends on the case,
555555
but it's usually a good idea to only handle the constant pattern first, and
556556
then generalize later if it seems useful.
557+
558+
## Guidelines for reviewers
559+
560+
* Do not ask contributors to implement non-splat vector support in code
561+
reviews. If you think non-splat vector support for a fold is both
562+
worthwhile and policy-compliant (that is, the handling would not result in
563+
any appreciable increase in code complexity), implement it yourself in a
564+
follow-up patch.

0 commit comments

Comments
 (0)