Based on D123467.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
In general I'm in favour of this sort of thing but only if they're logically grouped and we're not just trying to save lines of code. We should only be using these overloads when they increase comprehension: I stopped reviewing at a certain point as I felt I was becoming repetitive.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
466 | Maybe keep insert/extract separate? They're conceptually different enough to reductions to warrant their own actions. | |
475–476 | Again I'm not sure whether these all belong together and we're not saving much code by grouping them together. | |
489–491 | VP binary ops and reductions don't belong together, if you ask me. | |
577–579 | This is another big jumble of operations. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
210 | This should have curly braces because the else has curly braces. See the fourth example here https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements | |
241–242 | I think this comment should be with the BITREVERSE for i8 now since it would be part of the initializer list if it existed. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
241–242 | This comment needs to be above the previous line for it to make sense. It's explaining why the i8 doesn't use {ISD::BITREVERSE, ISD::BSWAP} |
LGTM
For the future, it would be nice to split up a patch like this into smaller pieces so that regressions (if any) are easier to identify. For something like this, getting consensus on direction and then checking in a series of patches without separate review would be fine.
This should have curly braces because the else has curly braces. See the fourth example here https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements