Page MenuHomePhabricator

[NFC][X86][CodeGen] Use ArrayRef in TargetLowering functions
AbandonedPublic

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

Details

Diff Detail

Event Timeline

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

This just makes the code much harder to decipher

Are you aware of any performance improvement from this change? The code is called so little I'm not expecting much benefit TBH

This just makes the code much harder to decipher

Are you aware of any performance improvement from this change? The code is called so little I'm not expecting much benefit TBH

I tend to agree as well. The flexibility of the API can be useful, but blanket changing to use the API as much as possible don't seem like a good idea. In the old style you could do a quick scan up and down to see which opcodes which opcodes and types were affected because we oftened lined things up in columns and ignored clang-format when it wanted to change it. With this change you have to scan across an entire line and sometimes across multiple lines to understand it and it makes visually comparing how different operations are handled harder.

I don't object as much to grouping things like the signed and unsigned form of an operation together. Or very related operations like add and sub. But changes that group a set of unrelated operations together just to make the file shorter I think goes too far.

RKSimon requested changes to this revision.May 1 2022, 8:13 AM

This needs a much lighter touch - even if it means deviating from some clang-format rules - to ensure the Opcodes and Types are easily distinguishable

This revision now requires changes to proceed.May 1 2022, 8:13 AM
sunshaoce updated this revision to Diff 427979.May 8 2022, 10:49 PM

Address comments

sunshaoce abandoned this revision.May 23 2022, 9:08 PM