[PATCH] D143759: [AMDGPU] Implement whole wave register spill

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIDefines.h:934
+enum Register_Flag : uint8_t {
+  WWM_REG = 0 // Register operand in a whole-wave mode operation.
+};
----------------
arsenm wrote:
> I assumed the value would be 1. Change this to be the pre-shifted values rather than using 1 << flag in the getter?
Will do.


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp:549
+
+void SIMachineFunctionInfo::MRI_NotecloneVirtualRegister(Register NewReg,
+                                                         Register SrcReg) {
----------------
arsenm wrote:
> Capitalization is weird, Capitalize clone
Will do.


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:651
+    assert(Reg.isVirtual());
+    if (VRegFlags.inBounds(Reg))
+      VRegFlags[Reg] |= (uint8_t)1 << Flag;
----------------
arsenm wrote:
> Why are the inbounds checks needed? The analagous MRI functions do not tolerate invalid register values 
VRegFlags will be grown each time (if the size exceeds the initial size set) when a new virtual register is created/cloned. The MRI's 'VRegToType' has a similar bound check.


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h:656-657
+  bool checkFlag(Register Reg, uint8_t Flag) const {
+    if (Reg.isPhysical())
+      return false;
+
----------------
arsenm wrote:
> This should assert (like setFlag already does)
I guess it's already been discussed in the downstream review. There are instances when Physical registers are taken to spill - fastAlloc, CSR spills during PEI. To handle such instances, an early return was inserted intentionally for the PhysRegs instead of an assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143759