[PATCH] D74873: [AMDGPU] Define 16 bit VGPR subregs

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/MC/LaneBitmask.h:39-79
   struct LaneBitmask {
     // When changing the underlying type, change the format string as well.
-    using Type = unsigned;
+    using Type = uint64_t;
     enum : unsigned { BitWidth = 8*sizeof(Type) };
-    constexpr static const char *const FormatStr = "%08X";
+    constexpr static const char *const FormatStr = "%016lX";
 
     constexpr LaneBitmask() = default;
----------------
This should be split to a separate change


================
Comment at: llvm/lib/CodeGen/MIRParser/MIParser.cpp:753-757
+      static_assert(sizeof(LaneBitmask::Type) == sizeof(uint64_t),
                     "Use correct get-function for lane mask");
       LaneBitmask::Type V;
-      if (getUnsigned(V))
+      if (getUint64(V))
         return error("invalid lane mask value");
----------------
Ditto


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:47
+
+  assert(getSubRegIndexLaneMask(AMDGPU::sub0).getAsInteger() == 3 &&
+         getSubRegIndexLaneMask(AMDGPU::sub31).getAsInteger() == (3ULL << 62) &&
----------------
Can this be a state_assert? I would hope getSubRegIndexLaneMask is constexpr


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.td:443
+
+def VGPR_HI16 : RegisterClass<"AMDGPU", Reg16Types.types, 16,
+                              (add (sequence "VGPR%u_HI16", 0, 255))> {
----------------
This should get a comment noting that there is no encoding for the high registers, and the low register are just encoded as the 32-bit register 


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

https://reviews.llvm.org/D74873