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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec 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:
> > 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
> You can take a look at D103213 which does that. I was able to get away w/o new operand types, but I still believe that is an overkill to do it that way.
And for that I had to write a pipe of code in the D103213 to handle it in the isOperandLegal(), which as I said is an overkill.


================
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.
----------------
arsenm wrote:
> The SSA check doesn't make sense, the class is unrelated
Any idea how to skip verifier after selection with gisel and before finalize lowering? We should have lot more problems like this as finalizeLowering() calls legalizeOperands() for many more instructions, we just do not have enough tests I guess.


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

https://reviews.llvm.org/D103197