[InstCombine] Do not request non-splat vector support in code reviews (NFC) #90709
Conversation
I take it this is aimed at me, and I'm sorry if this has caused you problems. Please can you give examples of the damaging combines? |
Looks like not wanting to point fingers too much backfired -- I'm referring to Roman Lebedev here, who religiously implemented full non-splat vector support for every single new fold. One specific example I remember is this one: Modifying that kind of code is really annoying -- even if you want to do something "trivial" like supporting an additional predicate. An alternative implementation using ConstantRange utilities could handle all predicates uniformly with a lot less code -- but of course, you can't support non-splat vectors that way... |
|
Thanks for the clarification! |
|
|
||
| ## Guidelines for reviewers | ||
|
|
||
| * Do not ask contributors to implement non-splat vector support in code |
There was a problem hiding this comment.
maybe this should be "new reviewers contributors". IIUC the motivation is that its complicated and may be too much for a novice.
There was a problem hiding this comment.
Done. The problem is not just that it can be complicated, but that it is likely to significantly increase the number of required review iterations or even result in directly contradictory review feedback. We don't have a strong consensus on what exactly is acceptable when implementing non-splat vector support or what the best way of going about it is.
There was a problem hiding this comment.
Hopefully #85676 will change that :)
… (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.
nikic
force-pushed
the
instcombine-non-splat
branch
from
f3bba85
to
1a04be3
Compare
The InstCombine contributor guide already says:
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.