[llvm] [AMDGPU][NFCI] Reorder AGPRs to allow skipping over them (PR #105633)

Matt Arsenault via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 22 05:23:42 PDT 2024


================
@@ -3365,6 +3365,24 @@ SIRegisterInfo::getConstrainedRegClassForOperand(const MachineOperand &MO,
   return nullptr;
 }
 
+unsigned SIRegisterInfo::getNumSupportedRegs(const MachineFunction &MF) const {
+#ifndef NDEBUG
+  for (unsigned K = AMDGPU::AGPR0; K < AMDGPU::NUM_TARGET_REGS; ++K) {
+    // Skip lo16 registers, they're "fake" and don't have a regclass assigned.
+    if (K >= AMDGPU::AGPR0_HI16 && K <= AMDGPU::AGPR255_HI16)
+      continue;
+    if (!isAGPR(MF.getRegInfo(), K))
+      report_fatal_error("register at index " + Twine(K) + " is not an AGPR!");
+  }
+#endif
+
+  // Don't include AGPRs on targets that don't have them.
+  // This cuts about 4000 register (almost half of all registers) off.
+  return MF.getInfo<SIMachineFunctionInfo>()->mayUseAGPRs(MF.getFunction())
----------------
arsenm wrote:

> This is worrying because if we have to always enable AGPRs on all GFX90AInsts

90a insts was the wrong one, ST.hasMAIInsts && usesAGPRs. On gfx90a+ we hardly use AGPRs. the IR attribute only detects inline asm usage. We only use AGPRs there for very low occupancy cases (and I think requires manually overriding with amdgpu-waves-per-eu to ever use them)

> Would the fix be easy? Is there an extra check this can do to avoid the issue?

I don't think it matters for your purposes, I've just been thinking about the meaning of "amdgpu-no-agprs" and it's possible misinterpretation, which you seem to have run into here. 

> I think this is targeted at reducing iteration, and the size of containers that are used for registers (arrays, BitVectors, etc.). If this lands, I'd like to see if we can getNumSupportedRegs in more places.

It's too coarse grained. What we really want is to shrink the size of all classes to avoid reserved cases (which is the main point of RegisterClassInfo, which should probably be used more). This patch is probably fine as-is, but it's more limited than It could be (and could break if the register enum values ever change)


https://github.com/llvm/llvm-project/pull/105633


More information about the llvm-commits mailing list