[PATCH] D78591: [AMDGPU] Define special SGPR subregs

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:266-267
 
+  for (auto Reg : AMDGPU::SReg_32RegClass) {
+    Reserved.set(getSubReg(Reg, AMDGPU::hi16));
+    Register Low = getSubReg(Reg, AMDGPU::lo16);
----------------
rampitec wrote:
> rampitec wrote:
> > arsenm wrote:
> > > rampitec wrote:
> > > > rampitec wrote:
> > > > > rampitec wrote:
> > > > > > arsenm wrote:
> > > > > > > rampitec wrote:
> > > > > > > > rampitec wrote:
> > > > > > > > > arsenm wrote:
> > > > > > > > > > Can't you just iterate SGPR_16LO and use reserveRegisterTuples?
> > > > > > > > > This will reserve all SGPRs, not saying that it will reserve SGPR_LO16 which is needed to be used.
> > > > > > > > Also the point is this is mostly not about SGPR, this is about SReg. Stuff like vcc_lo_hi16. Without reserving verifier complains about missing live-ins in some basic blocks.
> > > > > > > I mean I would expect there to be an SReg_16LO too 
> > > > > > Not really, these are needed. You materialize a 16 bit immediate into SGPR_LO16 so that it can be consumed by a 16 bit operand. Well at least this is a working concept so far.
> > > > > Look, that is the code I am going to use:
> > > > > 
> > > > > ```
> > > > >   if (Subtarget->has16BitInsts()) {
> > > > >     addRegisterClass(MVT::i16, &AMDGPU::SGPR_LO16RegClass);
> > > > >     addRegisterClass(MVT::f16, &AMDGPU::SGPR_LO16RegClass);
> > > > > ```
> > > > > For this SGPR_LO16 shall be available.
> > > > I see the source of confusion now. It is a wrong RC, should be !SGPR_LO16RegClass.contains(Low). Noticed it when a test run out of registers.
> > > I mean we're missing a register class definition. I would expect a parallel set of _16LO to go with the current SGPR+ sets of registers we already have
> > Do you mean you want to have SReg_LO16? So far I do not see a real use for it.
> It also will not help the logic in this loop.
Yes, otherwise there will be an asymmetry. Really SReg_LO16 should be the allocable form so VCC isn't special 


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

https://reviews.llvm.org/D78591