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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh 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);
+
----------------
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?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137542