Page MenuHomePhabricator

[AMDGPU] Base getSubRegFromChannel on TableGen data
ClosedPublic

Authored by critson on Sun, Oct 11, 10:14 PM.

Details

Summary

Generate (at runtime) the table used to drive getSubRegFromChannel,
base on AMDGPUSubRegIdxRanges from TableGen data.
The is a step closer to it being staticly generated by TableGen and
allows getSubRegFromChannel handle all bitwidths in the mean time.

Diff Detail

Event Timeline

critson created this revision.Sun, Oct 11, 10:14 PM
critson requested review of this revision.Sun, Oct 11, 10:14 PM

Looks good, I have one comment.

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
92–93

I think this should be just sizeof(SubRegFromChannelTable).
The previous code used AMDGPU::NoSubRegister instead of NoRegister (both are zero so technically there is no difference).

arsenm accepted this revision.Mon, Oct 12, 7:17 AM

LGTM. I think this can remove some of the workarounds in the globalisel insert/extract selection

This revision is now accepted and ready to land.Mon, Oct 12, 7:17 AM
arsenm added inline comments.Mon, Oct 12, 8:31 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
92–93

Yes, this should be NoSubregister

rampitec added inline comments.Mon, Oct 12, 12:34 PM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
45

This could use a comment.

critson updated this revision to Diff 297747.Mon, Oct 12, 7:51 PM

Address reviews comments:

  • Fix initialiser to use AMDGPU::NoSubRegister and not memset.
  • Add comment on mapping array.
critson marked 3 inline comments as done.Mon, Oct 12, 7:54 PM

Having addressed the comments could I get a second quick read before I submit?
Thanks.

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
45

Added a detailed comment.

92–93

Agreed, this was meant to be AMDGPU::NoSubRegister.
However I realised this should not be a memset because we are initialising multi-byte elements, so it will fail if AMDGPU::NoSubRegister is not 0.
Switch to an initialisation loop and leave the compiler to appropriately unroll this.

foad added a subscriber: foad.Tue, Oct 13, 9:08 AM
foad added inline comments.
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
45

It doesn't explain what the 9 means. 9-1 is not a valid index into SubRegFromChannelTable. Also you have a double full stop at the end.

92–93

If you use std::array you can iterate over them with range-based for loops, instead of using array_lengthof.

critson updated this revision to Diff 298008.Tue, Oct 13, 6:44 PM
critson marked 4 inline comments as done.
  • Use std::array and tidy up initialisation.
  • Fix number of rows in table.
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
45

Apparently I forgot to increase the array size and this magically did not create any problems. My assertions were also not tight enough to catch this.

foad accepted this revision.Wed, Oct 14, 12:14 AM

Looks good. Some nits inline.

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
49

uint16_t seems like an odd type to use. unsigned would be more normal, or uint8_t if you want to keep the size down. But it really doesn't matter.

97

You can use .size() here.

188–192

.size()

critson added inline comments.Wed, Oct 14, 12:30 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
49

Currently SubReg is allocated 12 bits in MachineOperand, so needs to be bigger than uint8_t, but fits nicely in uint16_t.

97

It's not a std::array.

188–192

It's not a std::array.

foad added inline comments.Wed, Oct 14, 12:34 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
49

I understand that argument for the ChannelTable but not for the WidthMap.

97

Sorry, I missed that. But it could be :)

critson updated this revision to Diff 298058.Wed, Oct 14, 12:41 AM
  • Use std::array for map array.
critson marked 5 inline comments as done.Wed, Oct 14, 12:44 AM
critson added inline comments.
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
49

Sorry I missed that and agree.

97

I'm a C programmer at heart, so I have an instinctive resistance to these C++ things. I have switched it to std::array.

foad accepted this revision.Wed, Oct 14, 2:03 AM
This revision was landed with ongoing or failed builds.Wed, Oct 14, 4:26 AM
This revision was automatically updated to reflect the committed changes.
critson marked 2 inline comments as done.
Flakebi added inline comments.Mon, Oct 19, 5:46 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
102

This index should also be Width - 1