[PATCH] D121418: [AMDGPU] Add symbolic names for gfx940 HWREGs

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Seems OK but maybe a bit over-complicated as noted inline.



================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:1083
+bool isValidHwreg(int64_t Id, const MCSubtargetInfo &STI, StringRef Name) {
+  if (isGFX10(STI) && Name == "HW_REG_HW_ID") // An alias
+    return true;
----------------
I don't understand why you need to handle this alias here and on line 1092 below, and the old code had no mention of it?


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:1104
+  if (Id == ID_HW_ID)
+    return isSI(STI) || isCI(STI) || isVI(STI) || isGFX9(STI);
+
----------------
We also return true for GFX10Plus above, so doesn't that mean that ID_HW_ID needs no special handling at all?


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

https://reviews.llvm.org/D121418