[PATCH] D33583: [AMDGPU] Allow SDWA in instructions with immediates and SGPRs

[Sam Kolton via Phabricator via llvm-commits]

SamWot added inline comments.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:590
 
-  // 2. Are all operands - VGPRs
-  for (const MachineOperand &Operand : MI.explicit_operands()) {
-    if (!Operand.isReg() || !TRI->isVGPR(*MRI, Operand.getReg()))
+  // 2. Are all operands - VGPRs or can be changed to VGPRs
+  const MCInstrDesc &Desc = TII->get(MI.getOpcode());
----------------
I think there should be some heuristic to check if this should be done. E.g. if we would fold only one SDWA operand in this instruction then creating this copy would only increase code size.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:596-597
+        (Operand.isReg() && !TRI->isVGPR(*MRI, Operand.getReg()))) {
+      if (Desc.OpInfo[I].RegClass == -1 ||
+         !TRI->hasVGPRs(TRI->getRegClass(Desc.OpInfo[I].RegClass)))
+        return false;
----------------
Why this check is needed? It seems redundant for me.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:731
+    else if (Op.isReg())
+      Copy.addReg(Op.getReg(), Op.isKill() ? RegState::Kill : 0);
+    Op.ChangeToRegister(VGPR, false);
----------------
You should check for subregs


Repository:
  rL LLVM

https://reviews.llvm.org/D33583