[PATCH] D41617: [AMDGPU] Add HW_REG_SH_MEM_BASES symbolic name for s_getreg_b32
Artem Tamazov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 16 01:29:12 PST 2018
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
More information about the llvm-commits
mailing list