This patch is similar to D122557, adding an ArrayRef version for setOperationAction, setLoadExtAction, setCondCodeAction, setLibcallName.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
2278 | It shouldn't be necessary to add four overloads. An ArrayRef<T> also accepts a single T, so just having the ArrayRef + ArrayRef variant should cover all cases. |
llvm/lib/CodeGen/TargetLoweringBase.cpp | ||
---|---|---|
210 | This is less readable. It’s not intuitive to me. |
llvm/lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
223 ↗ | (On Diff #421837) | I'm really not sure this kind of change makes the code easier to understand or manage, and I'd be surprised if there was a performance gain to be had. I'm not against adding the ArrayRef<> wrappers but I'd be a lot more careful about where they are used. |
llvm/lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
223 ↗ | (On Diff #421837) | To add my 2 cents, I think broadly speaking the increased use of ArrayRef is a minor readability improvement, making the setOperatinAction lines easier to read by better grouping + condensing related expansions. I see what you mean about this particular change not making a big difference...but it doesn't seem like it's any worse than before, and I can see the logic in making use of ArrayRefs wherever possible. |
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
2259 | Can't we have ArrayRef for both operands and use 2 for loops. Why do we need two functions? | |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
446 ↗ | (On Diff #421837) | This for loop variable name doesn't make sense. They aren't VTs they're opcodes |
llvm/lib/Target/X86/X86ISelLowering.cpp | ||
627 ↗ | (On Diff #421837) | There a logical grouping of sin/cos and conversion functions where the blank line was. This removes that grouping and now has a comment that only refers to the 3 of the things in this list. |
747 ↗ | (On Diff #421837) | I'm not sure this an improvement. It's one line longer and you have to look quite a bit further ahead to understand what it's doing. Maybe there's some middle ground of breaking this up into logical groups that would reduce the number of lines. Like putting all the CTPOIP/CTTZ/CTLZ in one group? @RKSimon what do you think? |
llvm/lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
747 ↗ | (On Diff #421837) | I'm really not keen on most of these changes tbh - it makes it a lot tricker to notice individual opcodes, and the types are even trickier to keep track of. I appreciate the attempt, but unless there's a bigger plan here that I'm missing I don't think this NFC tidyup is a good idea. |
How about just dealing with the TargetLowering.h / TargetLoweringBase.cpp changes in this patch and address the per-target changes individually?
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
2259 | llvm-project/llvm/lib/CodeGen/TargetLoweringBase.cpp:844:37: error: no viable conversion from 'llvm::MVT::SimpleValueType' to 'ArrayRef<llvm::MVT>' setOperationAction(ISD::PREFETCH, MVT::Other, Expand); It doesn't seem possible to convert both MVT to ArrayRef<MVT> at the same time, any better idea? |
Can't we have ArrayRef for both operands and use 2 for loops. Why do we need two functions?