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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec marked 2 inline comments as done.
rampitec 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);
----------------
arsenm wrote:
> 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 
Ok, I will add it. I am not sure it will help physreg livein into a BB thought. I have a patch somewhere which defines it, but I recall I have seen the same issue.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:269-270
+    Register Low = getSubReg(Reg, AMDGPU::lo16);
+    if (!AMDGPU::SGPR_LO16RegClass.contains(Low))
+      Reserved.set(Low);
+  }
----------------
arsenm wrote:
> Add a note that this is because there's no SReg_LO16 at least
I will add SReg_LO16 and see how it goes. Thanks.


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

https://reviews.llvm.org/D78591