[PATCH] D27846: [SLP] Support for horizontal min/max reduction

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

This is another example of what I was talking about re smaller patches.

You could do things like name changes (Reduction -> ArithmeticReduction) or moving comments around (from inside the body of getReductionCost to above the declaration) in separate NFC patches. In a lot of cases they're simple enough not to require pre-commit review, and it makes reviewing actual meaningful patches much much easier, because the diff only contains relevant things. You could also start with a patch that only supports one kind of min/max reduction, and add the rest of them as a follow-up.

I'm sorry if it seems like I'm being petty. In fact I'm really interested in getting all of this stuff in. But the SLP vectorizer is not the simplest or the clearest piece of code to begin with, and I'm not deeply familiar with its nuances, so it's really hard for me to meaningfully review SLP patches if they also contain noise, and if they do more than one thing. If there's someone else who's capable of reviewing and LGTMing these patches as is, I don't oppose it. But I can't.


https://reviews.llvm.org/D27846