[PATCH] D41617: [AMDGPU] Add HW_REG_SH_MEM_BASES symbolic name for s_getreg_b32

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:3168
+    if (isSI() || isCI() || isVI())
+      Last = ID_SYMBOLIC_FIRST_GFX9_;
+    for (int i = ID_SYMBOLIC_FIRST_; i < Last; ++i) {
----------------
artem.tamazov wrote:
> Just a recommendation:
> ```
> const int Last = (isSI() || isCI() || isVI()) ? ID_SYMBOLIC_LAST_PRIOR_GFX9_
>     : ID_SYMBOLIC_LAST_;
> ```
> Not ideal anyway, but `const` at least. Up to you.
With more values added in the future this expression would become hardly readable.


================
Comment at: lib/Target/AMDGPU/SIDefines.h:277
   ID_MEM_BASES = 15,
+  ID_SYMBOLIC_FIRST_GFX9_ = ID_MEM_BASES,
+  ID_SYMBOLIC_LAST_ = 16,
----------------
artem.tamazov wrote:
> artem.tamazov wrote:
> > Recommendation: I would rename `ID_SYMBOLIC_FIRST_GFX9_` to `ID_SYMBOLIC_LAST_PRIOR_GFX9_` or alike.
> Value should be set to 8, as it was prior these changes.
If it will be 8 the patch would not work. We shall recognize sh_mem_bases on GFX9.


================
Comment at: lib/Target/AMDGPU/SIDefines.h:277
   ID_MEM_BASES = 15,
+  ID_SYMBOLIC_FIRST_GFX9_ = ID_MEM_BASES,
+  ID_SYMBOLIC_LAST_ = 16,
----------------
rampitec wrote:
> artem.tamazov wrote:
> > artem.tamazov wrote:
> > > Recommendation: I would rename `ID_SYMBOLIC_FIRST_GFX9_` to `ID_SYMBOLIC_LAST_PRIOR_GFX9_` or alike.
> > Value should be set to 8, as it was prior these changes.
> If it will be 8 the patch would not work. We shall recognize sh_mem_bases on GFX9.
> ID_SYMBOLIC_LAST_PRIOR_GFX9_

To me that would be cryptic ;)


https://reviews.llvm.org/D41617