[PATCH] D103197: [AMDGPU] All GWS instructions need aligned VGPR on gfx90a

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:4225-4234
+      // Add implicit aligned super-reg to force alignment on the data operand.
+      const DebugLoc &DL = MI.getDebugLoc();
+      MachineRegisterInfo &MRI = BB->getParent()->getRegInfo();
+      const SIRegisterInfo *TRI = Subtarget->getRegisterInfo();
+      MachineOperand *Op = TII->getNamedOperand(MI, AMDGPU::OpName::data0);
+      Register DataReg = Op->getReg();
+      bool IsAGPR = TRI->isAGPR(MRI, DataReg);
----------------
rampitec wrote:
> arsenm wrote:
> > Should just constrain the register class to an even aligned 32-bit register class
> That was the first thing I did. It is much more code to handle just these 3 instructions, and it does not really work, I cannot prevent SIFoldOperands from replacing a register with unaligned one. Then isLegalRegOperand() should also handle this situation, which in turn would mean duplicating ds_gws opcodes with a different operand RC, handling new operand type in asm/disasm and other places.
> 
> On top of that such RC must have OtherVT as the only supported VT so that ISel will not use it after a call to getMinimalPhysRegClass(). This RC also will make it a quest to implement something line getCompatibleSubRegClass() and similar when an odd subreg is requested.
> 
> This is really an overkill.
SIFoldOperands should be respecting operand constraints


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:4351-4353
+    // Register class of these instructions is fixed in the finalize lowering,
+    // thus do not check it before we done with SSA which is guaranteed past
+    // that point.
----------------
The SSA check doesn't make sense, the class is unrelated


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

https://reviews.llvm.org/D103197