This is an archive of the discontinued LLVM Phabricator instance.

[NFC][RISCV][CodeGen] Use ArrayRef in TargetLowering functions
ClosedPublic

Authored by sunshaoce on Apr 12 2022, 10:29 PM.

Diff Detail

Event Timeline

sunshaoce created this revision.Apr 12 2022, 10:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 10:29 PM
sunshaoce requested review of this revision.Apr 12 2022, 10:29 PM

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.

sunshaoce updated this revision to Diff 422845.Apr 14 2022, 6:27 AM
sunshaoce marked 4 inline comments as done.

Address @frasercrmck's comments.

craig.topper added inline comments.Apr 17 2022, 9:40 PM
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.

sunshaoce updated this revision to Diff 423373.Apr 18 2022, 6:52 AM
sunshaoce marked 2 inline comments as done.

Address @craig.topper's comments

Looks reasonable to me, thanks for addressing my concerns

craig.topper added inline comments.Apr 25 2022, 7:30 PM
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}

reames accepted this revision.Apr 26 2022, 7:26 AM
reames added a subscriber: reames.

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 revision is now accepted and ready to land.Apr 26 2022, 7:26 AM
This revision was landed with ongoing or failed builds.Apr 26 2022, 8:53 AM
This revision was automatically updated to reflect the committed changes.