[PATCH] D137542: [AMDGPU] Use aperture registers instead of S_GETREG

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:149-150
+  // register with a reference to its artificial HI 32 bits.
+  if (IsApertureRegister64(SrcReg))
+    return RBI.constrainGenericRegister(DstReg, AMDGPU::SGPR_64RegClass, *MRI);
+
----------------
Pierre-vh wrote:
> arsenm wrote:
> > Pierre-vh wrote:
> > > arsenm wrote:
> > > > You shouldn't need to special case this (also note there's no equivalent for the DAG). Either it needs to belong to a different class from SReg_64, or you need to reserve the high bits 
> > > > or you need to reserve the high bits
> > > Aren't they reserved with `reserveRegisterTuples`?
> > > 
> > > If I create a different class, do you mean I should exclude those aperture registers from SReg and create a superclass with both SReg and the apertures? I'm not sure I understand how the class will help if I shouldn't special-case the uses of those aperture registers
> > If the reserve didn't do it, then yes. Using different classes is the correct way to represent use restrictions. Really what we want is a class without the sub1 subregister, but has a hack we're trying to avoid that, so a class that excludes the aperture regs is the next best thing.
> > 
> > Not sure if you really need the reserve after that.
> Do I need to do something similar to SRegOrLds, then?
>  
> Also:
> 
>   - Do I need to do this for 32 bit apertures or 64 bit apertures only?
>   - If I remove Aperture registers from SReg and create a superclass, won't I need to update instruction definitions to use that superclass instead of SReg? Won't that require a lot of changes?
> 
> And I suppose all those changes should be in a separate patch, right?
> 
> Do I need to do this for 32 bit apertures or 64 bit apertures only?
Well the 32-bit register cases are purely artificial. I'd assume these are included only by the broadest set of SGPRs (ideally wouldn't be in any allocatable set)

> If I remove Aperture registers from SReg and create a superclass, won't I need to update instruction definitions to use that superclass instead of SReg? Won't that require a lot of changes?
This is a question of naming. I'd probably lean towards leaving the SReg name/operands as-is, and introducing an allocatable subclass that excludes them (e.g. like SReg_64_XEXEC)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137542