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

[Carl Ritson via Phabricator via llvm-commits]

critson marked 3 inline comments as done.
critson added a comment.

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



================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:45
+
+static const uint16_t SubRegFromChannelTableWidthMap[17] = {
+    0, 1, 2, 3, 4, 5, 6, 7, 8, 0, 0, 0, 0, 0, 0, 0, 9};
----------------
rampitec wrote:
> This could use a comment.
Added a detailed comment.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:88-89
+  static auto InitializeSubRegFromChannelTableOnce = [this]() {
+    memset(SubRegFromChannelTable, AMDGPU::NoRegister,
+           sizeof(SubRegFromChannelTable) / sizeof(uint16_t));
+    for (uint16_t Idx = 1; Idx < getNumSubRegIndices(); ++Idx) {
----------------
arsenm wrote:
> Flakebi wrote:
> > 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).
> Yes, this should be NoSubregister 
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89217