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

[Artem Tamazov via Phabricator via llvm-commits]

artem.tamazov added a comment.

Let's reach consensus.



================
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:
> 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 ;)
I mean that `ID_SYMBOLIC_LAST_PRIOR_GFX9_` shall be set to 8 (which is currently named `ID_SYMBOLIC_FIRST_GFX9_`). That enum member is not used on Gfx9, as far as I see, and does not prevent ID_MEM_BASES from being recognized.

Am I wrong somewhere?


================
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:
> rampitec wrote:
> > 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 ;)
> I mean that `ID_SYMBOLIC_LAST_PRIOR_GFX9_` shall be set to 8 (which is currently named `ID_SYMBOLIC_FIRST_GFX9_`). That enum member is not used on Gfx9, as far as I see, and does not prevent ID_MEM_BASES from being recognized.
> 
> Am I wrong somewhere?
>>     ID_SYMBOLIC_LAST_PRIOR_GFX9_

> To me that would be cryptic ;)

To me, what is really cryptic is when `...FIRST...` is assigned to `Last` in lib/Target/AMDGPU/InstPrinter/AMDGPUInstPrinter.cpp#1259 and lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp#3168, like this:
```
unsigned Last = ID_SYMBOLIC_LAST_;
  if (AMDGPU::isSI(STI) || AMDGPU::isCI(STI) || AMDGPU::isVI(STI))
    Last = ID_SYMBOLIC_FIRST_GFX9_;
```



Repository:
  rL LLVM

https://reviews.llvm.org/D41617