[AMDGPU] gfx90a support

[kzhuravl]

LGTM

[tra]

This is a pretty huge patch, with no details in the commit log.

One hour between sending the patch out and landing it is not sufficient for anyone to meaningfully
review the patch and there are no mentions of the review done anywhere else.

While the code only changes AMDGPU back-end, it does not mean that the patch should be just rubber-stamped.

[rampitec]

In D96906#2570086, @tra wrote:

This is a pretty huge patch, with no details in the commit log.

One hour between sending the patch out and landing it is not sufficient for anyone to meaningfully
review the patch and there are no mentions of the review done anywhere else.

While the code only changes AMDGPU back-end, it does not mean that the patch should be just rubber-stamped.

It's a year of work necessarily downstream. Every line there was reviewed and tested in the downstream. I understand no one can reasonably review something that big, although I cannot break it into small patches after a year of changes and fixes. Not that I have too much choice.

[tra]

In D96906#2570095, @rampitec wrote:

It's a year of work necessarily downstream. Every line there was reviewed and tested in the downstream. I understand no one can reasonably review something that big, although I cannot break it into small patches after a year of changes and fixes. Not that I have too much choice.

The point is that nobody upstream even got a chance to chime in.

According to https://llvm.org/docs/CodeReview.html

We expect significant patches to be reviewed before being committed.

This patch certainly qualifies as significant.

[foad]

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id.ll

23

CO-V3 isn't tested by any RUN line. I think FileCheck might complain about this in future.

41

UNPACKED-TID isn't tested by any RUN line.

[kzhuravl]

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id.ll

23

Thanks for spotting this. I think issue was introduced in one of the merges from trunk. Fixed in https://reviews.llvm.org/D96967

41

Thanks for spotting this. I think issue was introduced in one of the merges from trunk. Fixed in https://reviews.llvm.org/D96967

[arsenm]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

191–199

This is a problem because I've removed forAllLanes.

This is a hack, we should be using a different register class for cases that don't support a given subregister index not scanning for an example non-reserved register

[rampitec]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

191–199

This would be massive duplication of all instructions with such operands, isn't it?

[kzhuravl]

The point is that nobody upstream even got a chance to chime in.

We are and will be taking care of any feedback provided in this review post-commit.

[msearles]

In D96906#2572749, @kzhuravl wrote:

The point is that nobody upstream even got a chance to chime in.

We are and will be taking care of any feedback provided in this review post-commit.

To be fair to @rampitec , it was not his desire to push this up in 1 big patch. We needed this upstreamed and no time was given to him to break it up into reasonably sized pieces. If it appears to be his doing/his intent, well, it should not. There have been a couple comments; I believe most addressed; comments will continue to be addressed.

[tra]

In D96906#2572842, @msearles wrote:

In D96906#2572749, @kzhuravl wrote:

The point is that nobody upstream even got a chance to chime in.

We are and will be taking care of any feedback provided in this review post-commit.

To be fair to @rampitec , it was not his desire to push this up in 1 big patch. We needed this upstreamed and no time was given to him to break it up into reasonably sized pieces. If it appears to be his doing/his intent, well, it should not. There have been a couple comments; I believe most addressed; comments will continue to be addressed.

Who landed the change is not particularly important.

What does matter is to make sure that shortcutting the review does not become a regular occurrence.

Stuff happens, Sometimes the standard rules may need to be bent. However, it should not be a unilateral decision by the committing side.

A better way to handle that would be to send the patch for review few days early (you presumably did have most/all of these changes made by then), provide details describing the changes (single subject line falls a bit short), outline your situation explaining why the patch can't be split and reviewed pre-commit. If the changes are indeed well-reviewed downstream, that would probably not pose much of a challenge to get the patch approved. If the patch does need further cleanups, at least we would have a reasonable idea how invasive they would be and could make an informed call. "Commit now, ask for forgiveness later" is not among the LLVM contribution guidelines, not for large patches. At the very minimum it should've been publicly discussed before the fact.

clang/include/clang/Driver/Options.td

3097–3101

We have BoolFOption to generate -fsomething and -fno-something

llvm/include/llvm/BinaryFormat/ELF.h

737

Nit: This looks odd. GFX90A does not need to be in the middle of the list. It makes it somewhat confusing to tell which ID is really the last. The _LAST enum says it's GFX90A, but it's not the last item of the list. There are already out-of-name-order GPUs at the end of the list, so putting 90A at the end would probably be a better choice. At least we'd still have the numeric values in order. Right now the list is ordered neither by the name nor by the value.

There's also a question of whether something needs to be done about the missing values 0x3c..0x3e. Presumably the _FIRST.._LAST enums specify the range we'll use to iterate over the GPU IDs. Do we handle the missing values correctly?

Looks like it's benign at the moment as we're only using it to return amdgcn triple in ELFObjectFile.h. I'd add the placeholder enums for the reserved/unused values within the range.

[echristo]

In D96906#2572842, @msearles wrote:

In D96906#2572749, @kzhuravl wrote:

The point is that nobody upstream even got a chance to chime in.

We are and will be taking care of any feedback provided in this review post-commit.

To be fair to @rampitec , it was not his desire to push this up in 1 big patch. We needed this upstreamed and no time was given to him to break it up into reasonably sized pieces. If it appears to be his doing/his intent, well, it should not. There have been a couple comments; I believe most addressed; comments will continue to be addressed.

"we needed this upstream" is a business issue on AMD's side, not an issue for the llvm project. In general the expectation is that code is reviewed according to the guidelines and a single reviewer with one (small) patch that wasn't a revert doesn't feel like sufficient review for something of this size. For something this size I'd have expected Matt to at least be on the reviewer line and that also wasn't done. This feels like an abuse of the review system and probably should be reverted.

Thanks.

-eric

[kzhuravl]

llvm/include/llvm/BinaryFormat/ELF.h

737

https://reviews.llvm.org/D97010

[kzhuravl]

clang/include/clang/Driver/Options.td

3097–3101

The reason we use m_amdgpu_Features_Group is it is getting transformed to target features (-mtgsplit to +tgsplit, mno-tgsplit to -tgsplit. For example, tgsplit target feature in AMDGPU backend:

https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/AMDGPU.td#L157

Does BoolFOption get translated to target features as well?

[rampitec]

clang/include/clang/Driver/Options.td

3097–3101

We could probably create similar BoolMOption. This is not only tgsplit, there are plenty of such binary options around.

[kzhuravl]

clang/include/clang/Driver/Options.td

3097–3101

Quickly glancing over Options.td, BoolFOption is in f_group, and does not get automatically converted to target-features

[kzhuravl]

clang/include/clang/Driver/Options.td

3097–3101

agreed, seems like a good choice given there is BoolFOption, BoolGOption

[kzhuravl]

clang/include/clang/Driver/Options.td

3097–3101

but it will still need to be in m_amdgpu_Features_Group because https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/AMDGPU.cpp#L403 unless we want to switch away from that. so maybe make a group an optional, last template parameter to BoolMOption?

[kzhuravl]

clang/include/clang/Driver/Options.td

3097–3101

Other targets also have its own corresponding groups: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td#L151

[rampitec]

clang/include/clang/Driver/Options.td

3097–3101

Looks like. It is not just the same thing as BoolFOption or BoolGOption.

[kzhuravl]

clang/include/clang/Driver/Options.td

3097–3101

@tra , do you have any suggestion based on the comments above? thanks.

[foad]

In D96906#2573265, @echristo wrote:

In D96906#2572842, @msearles wrote:

In D96906#2572749, @kzhuravl wrote:

The point is that nobody upstream even got a chance to chime in.

We are and will be taking care of any feedback provided in this review post-commit.

To be fair to @rampitec , it was not his desire to push this up in 1 big patch. We needed this upstreamed and no time was given to him to break it up into reasonably sized pieces. If it appears to be his doing/his intent, well, it should not. There have been a couple comments; I believe most addressed; comments will continue to be addressed.

"we needed this upstream" is a business issue on AMD's side, not an issue for the llvm project. In general the expectation is that code is reviewed according to the guidelines and a single reviewer with one (small) patch that wasn't a revert doesn't feel like sufficient review for something of this size. For something this size I'd have expected Matt to at least be on the reviewer line and that also wasn't done. This feels like an abuse of the review system and probably should be reverted.

Thanks.

-eric

I'd appreciate it if you could find a solution that does not involve reverting and reapplying later, as this will triple the amount of churn we get downstream. (I realise LLVM policy is not to care about downstream but I thought I'd plead my case anyway!)

[rampitec]

clang/include/clang/Driver/Options.td

3097–3101

I have created new BoolMOption here: D97069. Not sure if it saves much code but let's see if we like it.

[arsenm]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

191–199

Ideally yes. We can still use register classes for this, with special care to make sure we never end up with the unaligned virtual registers in the wrong contexts.

The less that's tracked by the instruction definitions, the more special case code we have to right. I've been thinking of swapping out the entire MCInstrDesc table per-subtarget to make this easier, although that may be a painful change.

[rampitec]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

191–199

I do not see how it can be less code. You will need to duplicate all VALU pseudos, not just real instructions. Which means every time you write in the source something like AMDGPU::FLAT_LOAD_DWORDX2 you would have to write an if. For every VALU instruction.

[arsenm]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

191–199

It's less code because the code that's already there is supposed to rely on the static operand definitions. Every time we want to deviate from those, we end up writing manual code in the verifier and fixup things here and there that differ.

The point of swapping out the table would be to eliminate all the VALU pseudos. We would just have the same enum values referring to different physical instruction definitions

[rampitec]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

191–199

This makes sense, although as you said also quite painful and to me also sounds like a hack. There is still a lot of legalization needed even with this approach. Every time you hit an instruction not supported by a target you will need to do something about it. In a worst case expanding. Sounds like another year of work. Especially when you look at highly specialized ASICs which can do this but cannot do that, and you have a lot them.

[rampitec]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

191–199

JBTW it will not help anyway. Not for this problem. You may create an operand of a different RC or you may just reserve every other register like I did, the net result will be the same, you will end up using prohibited register. Imagine you are using an RC where only even tuples are added. And then you are using sub1_sub2 subreg of it. RA will happily allocate forbidden register just like it does now. To me this is RA bug in the first place to allocate a reserved register.

The only thing which could help is an another register info without odd wide subregs, but that you cannot achieve just by duplication of instruction definitions, for that you would need to duplicate register info as well. This is almost a new BE.

[arsenm]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

191–199

It's not a hack, this is how operand classes are intended to work. You wouldn't be producing these instructions on targets that don't support them (ideally we would also have a verifier for this, which is another area where subtarget handling is weak).

The point is to not reserve them. References to unaligned registers can exist, they just can't be used in the context of a real machine operand. D97316 switches to using dedicated classes for alignment (the further cleanup would be to have this come directly from the instruction definitions instead of fixing them up after isel).

[rampitec]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

191–199

It's not a hack, this is how operand classes are intended to work. You wouldn't be producing these instructions on targets that don't support them (ideally we would also have a verifier for this, which is another area where subtarget handling is weak).

The point is to not reserve them. References to unaligned registers can exist, they just can't be used in the context of a real machine operand. D97316 switches to using dedicated classes for alignment (the further cleanup would be to have this come directly from the instruction definitions instead of fixing them up after isel).

I have replied in D97316, but I do not believe it will help as is. It will run into the exactly same issue as with reserved registers approach and their subregs.

[rampitec]

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

191–199

I have checked the commit. This code is not even related to vgpr agignment. It was about SGPRs in fact:

If stack is present 4 low SGPRs are reserved. The pass was
using just a first register as a representative for RC to
check for reserved subregs. The first register was reserved
and pass failed to find a valid lane split.

The testcase is mem_clause_sreg256_used_stack from memory_clause.mir.

[foad]

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

100

Spelling "sequence".

[rampitec]

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

100

Thanks! https://reviews.llvm.org/rG619b88849e14

clang/include/clang/Driver/Options.td

   HelpText<"Specify CU (-mcumode) or WGP (-mno-cumode) wavefront execution mode (AMDGPU only)">;
 def mno_cumode : Flag<["-"], "mno-cumode">, Group<m_amdgpu_Features_Group>;
 
+def mtgsplit : Flag<["-"], "mtgsplit">, Group<m_amdgpu_Features_Group>,
+  HelpText<"Enable threadgroup split execution mode (AMDGPU only)">;
+def mno_tgsplit : Flag<["-"], "mno-tgsplit">, Group<m_amdgpu_Features_Group>,
+  HelpText<"Disable threadgroup split execution mode (AMDGPU only)">;
+
 def mwavefrontsize64 : Flag<["-"], "mwavefrontsize64">, Group<m_Group>,
   HelpText<"Specify wavefront size 64 mode (AMDGPU only)">;
 def mno_wavefrontsize64 : Flag<["-"], "mno-wavefrontsize64">, Group<m_Group>,

llvm/include/llvm/BinaryFormat/ELF.h

   EF_AMDGPU_MACH_AMDGCN_GFX906        = 0x02f,
   EF_AMDGPU_MACH_AMDGCN_GFX908        = 0x030,
   EF_AMDGPU_MACH_AMDGCN_GFX909        = 0x031,
+  EF_AMDGPU_MACH_AMDGCN_GFX90A        = 0x03f,
   EF_AMDGPU_MACH_AMDGCN_GFX90C        = 0x032,
   EF_AMDGPU_MACH_AMDGCN_GFX1010       = 0x033,
   EF_AMDGPU_MACH_AMDGCN_GFX1011       = 0x034,

 
   // First/last AMDGCN-based processors.
   EF_AMDGPU_MACH_AMDGCN_FIRST = EF_AMDGPU_MACH_AMDGCN_GFX600,
-  EF_AMDGPU_MACH_AMDGCN_LAST = EF_AMDGPU_MACH_AMDGCN_GFX805,
+  EF_AMDGPU_MACH_AMDGCN_LAST = EF_AMDGPU_MACH_AMDGCN_GFX90A,
 
   // Indicates if the "xnack" target feature is enabled for all code contained
   // in the object.

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

 
   std::pair<const MachineOperand *, int> isOMod(const MachineInstr &MI) const;
   bool tryFoldOMod(MachineInstr &MI);
+  bool tryFoldRegSeqence(MachineInstr &MI);
+  bool tryFoldLCSSAPhi(MachineInstr &MI);
+  bool tryFoldLoad(MachineInstr &MI);
 
 public:
   SIFoldOperands() : MachineFunctionPass(ID) {

     return AMDGPU::V_FMA_F16_gfx9_e64;
   case AMDGPU::V_FMAC_LEGACY_F32_e64:
     return AMDGPU::V_FMA_LEGACY_F32_e64;
+  case AMDGPU::V_FMAC_F64_e64:
+    return AMDGPU::V_FMA_F64_e64;
   }
   return AMDGPU::INSTRUCTION_LIST_END;
 }

     return false;
 
   uint8_t OpTy = OpInfo[UseOpIdx].OperandType;
-  if (OpTy < AMDGPU::OPERAND_REG_INLINE_AC_FIRST ||
-      OpTy > AMDGPU::OPERAND_REG_INLINE_AC_LAST)
+  if ((OpTy < AMDGPU::OPERAND_REG_INLINE_AC_FIRST ||
+       OpTy > AMDGPU::OPERAND_REG_INLINE_AC_LAST) &&
+      (OpTy < AMDGPU::OPERAND_REG_INLINE_C_FIRST ||
+       OpTy > AMDGPU::OPERAND_REG_INLINE_C_LAST))
     return false;
 
   if (OpToFold.isImm() && TII->isInlineConstant(OpToFold, OpTy) &&

     return false;
 
   MachineRegisterInfo &MRI = UseMI->getParent()->getParent()->getRegInfo();
+
+  // Maybe it is just a COPY of an immediate itself.
+  MachineInstr *Def = MRI.getUniqueVRegDef(UseReg);
+  MachineOperand &UseOp = UseMI->getOperand(UseOpIdx);
+  if (!UseOp.getSubReg() && Def && TII->isFoldableCopy(*Def)) {
+    MachineOperand &DefOp = Def->getOperand(1);
+    if (DefOp.isImm() && TII->isInlineConstant(DefOp, OpTy) &&
+        TII->isOperandLegal(*UseMI, UseOpIdx, &DefOp)) {
+      UseMI->getOperand(UseOpIdx).ChangeToImmediate(DefOp.getImm());
+      return true;
+    }
+  }
+
   SmallVector<std::pair<MachineOperand*, unsigned>, 32> Defs;
   if (!getRegSeqInit(Defs, UseReg, OpTy, TII, MRI))
     return false;

       else if (TRI->isVGPR(*MRI, UseMI->getOperand(0).getReg()) &&
                TRI->isAGPR(*MRI, UseMI->getOperand(1).getReg()))
         UseMI->setDesc(TII->get(AMDGPU::V_ACCVGPR_READ_B32_e64));
+      else if (ST->hasGFX90AInsts() &&
+               TRI->isAGPR(*MRI, UseMI->getOperand(0).getReg()) &&
+               TRI->isAGPR(*MRI, UseMI->getOperand(1).getReg()))
+        UseMI->setDesc(TII->get(AMDGPU::V_ACCVGPR_MOV_B32));
       return;
     }
 

   return true;
 }
 
+// Try to fold a reg_sequence with vgpr output and agpr inputs into an
+// instruction which can take an agpr. So far that means a store.
+bool SIFoldOperands::tryFoldRegSeqence(MachineInstr &MI) {
+  assert(MI.isRegSequence());
+  auto Reg = MI.getOperand(0).getReg();
+
+  if (!ST->hasGFX90AInsts() || !TRI->isVGPR(*MRI, Reg) ||
+      !MRI->hasOneNonDBGUse(Reg))
+    return false;
+
+  SmallVector<std::pair<MachineOperand*, unsigned>, 32> Defs;
+  if (!getRegSeqInit(Defs, Reg, MCOI::OPERAND_REGISTER, TII, *MRI))
+    return false;
+
+  for (auto &Def : Defs) {
+    const auto *Op = Def.first;
+    if (!Op->isReg())
+      return false;
+    if (TRI->isAGPR(*MRI, Op->getReg()))
+      continue;
+    // Maybe this is a COPY from AREG
+    const MachineInstr *SubDef = MRI->getUniqueVRegDef(Op->getReg());
+    if (!SubDef || !SubDef->isCopy() || SubDef->getOperand(1).getSubReg())
+      return false;
+    if (!TRI->isAGPR(*MRI, SubDef->getOperand(1).getReg()))
+      return false;
+  }
+
+  MachineOperand *Op = &*MRI->use_nodbg_begin(Reg);
+  MachineInstr *UseMI = Op->getParent();
+  while (UseMI->isCopy() && !Op->getSubReg()) {
+    Reg = UseMI->getOperand(0).getReg();
+    if (!TRI->isVGPR(*MRI, Reg) || !MRI->hasOneNonDBGUse(Reg))
+      return false;
+    Op = &*MRI->use_nodbg_begin(Reg);
+    UseMI = Op->getParent();
+  }
+
+  if (Op->getSubReg())
+    return false;
+
+  unsigned OpIdx = Op - &UseMI->getOperand(0);
+  const MCInstrDesc &InstDesc = UseMI->getDesc();
+  const MCOperandInfo &OpInfo = InstDesc.OpInfo[OpIdx];
+  switch (OpInfo.RegClass) {
+  case AMDGPU::AV_32RegClassID:  LLVM_FALLTHROUGH;
+  case AMDGPU::AV_64RegClassID:  LLVM_FALLTHROUGH;
+  case AMDGPU::AV_96RegClassID:  LLVM_FALLTHROUGH;
+  case AMDGPU::AV_128RegClassID: LLVM_FALLTHROUGH;
+  case AMDGPU::AV_160RegClassID:
+    break;
+  default:
+    return false;
+  }
+
+  const auto *NewDstRC = TRI->getEquivalentAGPRClass(MRI->getRegClass(Reg));
+  auto Dst = MRI->createVirtualRegister(NewDstRC);
+  auto RS = BuildMI(*MI.getParent(), MI, MI.getDebugLoc(),
+                    TII->get(AMDGPU::REG_SEQUENCE), Dst);
+
+  for (unsigned I = 0; I < Defs.size(); ++I) {
+    MachineOperand *Def = Defs[I].first;
+    Def->setIsKill(false);
+    if (TRI->isAGPR(*MRI, Def->getReg())) {
+      RS.add(*Def);
+    } else { // This is a copy
+      MachineInstr *SubDef = MRI->getUniqueVRegDef(Def->getReg());
+      SubDef->getOperand(1).setIsKill(false);
+      RS.addReg(SubDef->getOperand(1).getReg(), 0, Def->getSubReg());
+    }
+    RS.addImm(Defs[I].second);
+  }
+
+  Op->setReg(Dst);
+  if (!TII->isOperandLegal(*UseMI, OpIdx, Op)) {
+    Op->setReg(Reg);
+    RS->eraseFromParent();
+    return false;
+  }
+
+  LLVM_DEBUG(dbgs() << "Folded " << *RS << " into " << *UseMI << '\n');
+
+  return true;
+}
+
+// Try to hoist an AGPR to VGPR copy out of the loop across a LCSSA PHI.
+// This should allow folding of an AGPR into a consumer which may support it.
+// I.e.:
+//
+// loop:                             // loop:
+//   %1:vreg = COPY %0:areg          // exit:
+// exit:                          => //   %1:areg = PHI %0:areg, %loop
+//   %2:vreg = PHI %1:vreg, %loop    //   %2:vreg = COPY %1:areg
+bool SIFoldOperands::tryFoldLCSSAPhi(MachineInstr &PHI) {
+  assert(PHI.isPHI());
+
+  if (PHI.getNumExplicitOperands() != 3) // Single input LCSSA PHI
+    return false;
+
+  Register PhiIn = PHI.getOperand(1).getReg();
+  Register PhiOut = PHI.getOperand(0).getReg();
+  if (PHI.getOperand(1).getSubReg() ||
+      !TRI->isVGPR(*MRI, PhiIn) || !TRI->isVGPR(*MRI, PhiOut))
+    return false;
+
+  // A single use should not matter for correctness, but if it has another use
+  // inside the loop we may perform copy twice in a worst case.
+  if (!MRI->hasOneNonDBGUse(PhiIn))
+    return false;
+
+  MachineInstr *Copy = MRI->getUniqueVRegDef(PhiIn);
+  if (!Copy || !Copy->isCopy())
+    return false;
+
+  Register CopyIn = Copy->getOperand(1).getReg();
+  if (!TRI->isAGPR(*MRI, CopyIn) || Copy->getOperand(1).getSubReg())
+    return false;
+
+  const TargetRegisterClass *ARC = MRI->getRegClass(CopyIn);
+  Register NewReg = MRI->createVirtualRegister(ARC);
+  PHI.getOperand(1).setReg(CopyIn);
+  PHI.getOperand(0).setReg(NewReg);
+
+  MachineBasicBlock *MBB = PHI.getParent();
+  BuildMI(*MBB, MBB->getFirstNonPHI(), Copy->getDebugLoc(),
+          TII->get(AMDGPU::COPY), PhiOut)
+    .addReg(NewReg, RegState::Kill);
+  Copy->eraseFromParent(); // We know this copy had a single use.
+
+  LLVM_DEBUG(dbgs() << "Folded " << PHI << '\n');
+
+  return true;
+}
+
+// Attempt to convert VGPR load to an AGPR load.
+bool SIFoldOperands::tryFoldLoad(MachineInstr &MI) {
+  assert(MI.mayLoad());
+  if (!ST->hasGFX90AInsts() || !MI.getNumOperands())
+    return false;
+
+  MachineOperand &Def = MI.getOperand(0);
+  if (!Def.isDef())
+    return false;
+
+  Register DefReg = Def.getReg();
+
+  if (DefReg.isPhysical() || !TRI->isVGPR(*MRI, DefReg))
+    return false;
+
+  SmallVector<const MachineInstr*, 8> Users;
+  SmallVector<Register, 8> MoveRegs;
+  for (const MachineInstr &I : MRI->use_nodbg_instructions(DefReg)) {
+    Users.push_back(&I);
+  }
+  if (Users.empty())
+    return false;
+
+  // Check that all uses a copy to an agpr or a reg_sequence producing an agpr.
+  while (!Users.empty()) {
+    const MachineInstr *I = Users.pop_back_val();
+    if (!I->isCopy() && !I->isRegSequence())
+      return false;
+    Register DstReg = I->getOperand(0).getReg();
+    if (TRI->isAGPR(*MRI, DstReg))
+      continue;
+    MoveRegs.push_back(DstReg);
+    for (const MachineInstr &U : MRI->use_nodbg_instructions(DstReg)) {
+      Users.push_back(&U);
+    }
+  }
+
+  const TargetRegisterClass *RC = MRI->getRegClass(DefReg);
+  MRI->setRegClass(DefReg, TRI->getEquivalentAGPRClass(RC));
+  if (!TII->isOperandLegal(MI, 0, &Def)) {
+    MRI->setRegClass(DefReg, RC);
+    return false;
+  }
+
+  while (!MoveRegs.empty()) {
+    Register Reg = MoveRegs.pop_back_val();
+    MRI->setRegClass(Reg, TRI->getEquivalentAGPRClass(MRI->getRegClass(Reg)));
+  }
+
+  LLVM_DEBUG(dbgs() << "Folded " << MI << '\n');
+
+  return true;
+}
+
 bool SIFoldOperands::runOnMachineFunction(MachineFunction &MF) {
   if (skipFunction(MF.getFunction()))
     return false;

 
       tryFoldInst(TII, &MI);
 
+      if (MI.isRegSequence() && tryFoldRegSeqence(MI))
+        continue;
+
+      if (MI.isPHI() && tryFoldLCSSAPhi(MI))
+        continue;
+
+      if (MI.mayLoad() && tryFoldLoad(MI))
+        continue;
+
       if (!TII->isFoldableCopy(MI)) {
         // Saw an unknown clobber of m0, so we no longer know what it is.
         if (CurrentKnownM0Val && MI.modifiesRegister(AMDGPU::M0, TRI))

llvm/lib/Target/AMDGPU/SIFormMemoryClauses.cpp

     return MaskA.getHighestLane() > MaskB.getHighestLane();
   });
 
+  MCRegister RepReg;
+  for (MCRegister R : *MRI->getRegClass(Reg)) {
+    if (!MRI->isReserved(R)) {
+      RepReg = R;
+      break;
+    }
+  }
+  if (!RepReg)
+    llvm_unreachable("Failed to find required allocatable register");
+
   for (unsigned Idx : CoveringSubregs) {
     LaneBitmask SubRegMask = TRI->getSubRegIndexLaneMask(Idx);
     if ((SubRegMask & ~LaneMask).any() || (SubRegMask & LaneMask).none())
       continue;
 
+    if (MRI->isReserved(TRI->getSubReg(RepReg, Idx)))
+      continue;
+
     Func(Idx);
     LaneMask &= ~SubRegMask;
     if (LaneMask.none())

   // tracking does not account for the alignment requirements for SGPRs, or the
   // fragmentation of registers the allocator will need to satisfy.
   if (Occupancy >= MFI->getMinAllowedOccupancy() &&
-      MaxPressure.getVGPRNum() <= MaxVGPRs / 2 &&
+      MaxPressure.getVGPRNum(ST->hasGFX90AInsts()) <= MaxVGPRs / 2 &&
       MaxPressure.getSGPRNum() <= MaxSGPRs / 2) {
     LastRecordedOccupancy = Occupancy;
     return true;

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.workitem.id.ll

 ; RUN: llc -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck --check-prefixes=ALL,MESA %s
 ; RUN: llc -mtriple=amdgcn-unknown-mesa3d -mcpu=tahiti -verify-machineinstrs < %s | FileCheck -check-prefixes=ALL,CO-V2 %s
 ; RUN: llc -mtriple=amdgcn-unknown-mesa3d -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefixes=ALL,CO-V2 %s
+; RUN: llc -march=amdgcn -mtriple=amdgcn-unknown-amdhsa -mcpu=gfx90a -verify-machineinstrs < %s | FileCheck -check-prefixes=ALL,PACKED-TID %s
 
 declare i32 @llvm.amdgcn.workitem.id.x() #0
 declare i32 @llvm.amdgcn.workitem.id.y() #0

 ; CO-V2: enable_vgpr_workitem_id = 0
 
 ; ALL-NOT: v0
-; ALL: {{buffer|flat}}_store_dword {{.*}}v0
+; ALL: {{buffer|flat|global}}_store_dword {{.*}}v0
+
+; CO-V3: .amdhsa_system_vgpr_workitem_id 0
+; PACKED-TID: .amdhsa_system_vgpr_workitem_id 0
 define amdgpu_kernel void @test_workitem_id_x(i32 addrspace(1)* %out) #1 {
   %id = call i32 @llvm.amdgcn.workitem.id.x()
   store i32 %id, i32 addrspace(1)* %out

 ; ALL-LABEL: {{^}}test_workitem_id_y:
 ; CO-V2: enable_vgpr_workitem_id = 1
 
-; ALL-NOT: v1
-; ALL: {{buffer|flat}}_store_dword {{.*}}v1
+; PACKED-TID: v_bfe_u32 [[ID:v[0-9]+]], v0, 10, 10
+; PACKED-TID: {{buffer|flat|global}}_store_dword {{.*}}[[ID]]
+
+; UNPACKED-TID-NOT: v1
+; UNPACKED-TID: {{buffer|flat}}_store_dword {{.*}}v1
+
+; CO-V3: .amdhsa_system_vgpr_workitem_id 1
+; PACKED-TID: .amdhsa_system_vgpr_workitem_id 1
 define amdgpu_kernel void @test_workitem_id_y(i32 addrspace(1)* %out) #1 {
   %id = call i32 @llvm.amdgcn.workitem.id.y()
   store i32 %id, i32 addrspace(1)* %out

 ; ALL-LABEL: {{^}}test_workitem_id_z:
 ; CO-V2: enable_vgpr_workitem_id = 2
 
-; ALL-NOT: v2
-; ALL: {{buffer|flat}}_store_dword {{.*}}v2
+; PACKED-TID: v_bfe_u32 [[ID:v[0-9]+]], v0, 20, 10
+; PACKED-TID: {{buffer|flat|global}}_store_dword {{.*}}[[ID]]
+
+; UNPACKED-TID-NOT: v2
+; UNPACKED-TID: {{buffer|flat}}_store_dword {{.*}}v2
+
+; CO-V3: .amdhsa_system_vgpr_workitem_id 2
+; PACKED-TID: .amdhsa_system_vgpr_workitem_id 2
 define amdgpu_kernel void @test_workitem_id_z(i32 addrspace(1)* %out) #1 {
   %id = call i32 @llvm.amdgcn.workitem.id.z()
   store i32 %id, i32 addrspace(1)* %out