Page MenuHomePhabricator

[PPC][CodeGen][NFC] Use ArrayRef in TargetLowering functions
Needs ReviewPublic

Authored by Miss_Grape on Apr 13 2022, 1:35 AM.

Details

Reviewers
sunshaoce
stefanp
nemanjai
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

Miss_Grape created this revision.Apr 13 2022, 1:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2022, 1:35 AM
Miss_Grape requested review of this revision.Apr 13 2022, 1:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2022, 1:35 AM
lkail added a reviewer: Restricted Project.Apr 13 2022, 2:25 AM
lkail added a subscriber: lkail.Apr 13 2022, 2:40 AM
lkail added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
197

I doubt if it really eases developer to read the code. This API implies Cartesian product, but some might think it as one-to-one mapping at first glance due to the same length.
Also, if we have new operation but only applied to MVT::f64, we still need another statement to specify it.

Miss_Grape added inline comments.Apr 13 2022, 3:32 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
197

Do you mean to change only one semantic, operate Ops alone or operate VTs alones??
Pleas see this : https://reviews.llvm.org/D123467

While I certainly appreciate the work to refactor this behemoth of a function, I am about as lukewarm on how much of a readability improvement this is. If all the targets are being similarly refactored, I won't stand in the way of doing the same to PPC. So if you get the approval for the other targets, we'll go ahead with PPC as well.

While I certainly appreciate the work to refactor this behemoth of a function, I am about as lukewarm on how much of a readability improvement this is. If all the targets are being similarly refactored, I won't stand in the way of doing the same to PPC. So if you get the approval for the other targets, we'll go ahead with PPC as well.

RISCV/X86 get the approval.