[PATCH] D89217: [AMDGPU] Base getSubRegFromChannel on TableGen data

[Carl Ritson via Phabricator via llvm-commits]

critson marked 5 inline comments as done.
critson added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:49
+//      meaning index 7 in SubRegFromChannelTable.
+static const uint16_t SubRegFromChannelTableWidthMap[17] = {
+    0, 1, 2, 3, 4, 5, 6, 7, 8, 0, 0, 0, 0, 0, 0, 0, 9};
----------------
foad wrote:
> critson wrote:
> > foad wrote:
> > > `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.
> > Currently SubReg is allocated 12 bits in MachineOperand, so needs to be bigger than uint8_t, but fits nicely in uint16_t.
> I understand that argument for the ChannelTable but not for the WidthMap.
Sorry I missed that and agree.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:97
+      unsigned Offset = AMDGPUSubRegIdxRanges[Idx].Offset / 32;
+      assert(Width < array_lengthof(SubRegFromChannelTableWidthMap));
+      Width = SubRegFromChannelTableWidthMap[Width];
----------------
foad wrote:
> critson wrote:
> > foad wrote:
> > > You can use `.size()` here.
> > It's not a std::array.
> Sorry, I missed that. But it could be :)
I'm a C programmer at heart, so I have an instinctive resistance to these C++ things.  I have switched it to std::array.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89217/new/

https://reviews.llvm.org/D89217