[PATCH] D108830: [AMDGPU] Propagate defining src reg for AGPR to AGPR Copys

[Vang Thao via Phabricator via llvm-commits]

vangthao added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp:96
+    case AMDGPU::V_ACCVGPR_WRITE_B32_e64:
+      break;
+    case AMDGPU::COPY: {
----------------
rampitec wrote:
> I do not think you need this case. This is a single 32 bit register, it will have a single def.
This may not necessarily be a single def. This may contain multiple defs if subregs are used. For example in the case that I am looking at:

undef %1.sub0:areg_128 = V_ACCVGPR_WRITE_B32_e64 %0.sub0:vreg_128, implicit $exec
%1.sub1:areg_128 = COPY %1.sub0:areg_128
%1.sub2:areg_128 = COPY %1.sub0:areg_128
%1.sub3:areg_128 = COPY %1.sub0:areg_128

These subregs are considered to be under %1 since def_instruction() does not consider different subregs as far as I can tell. I think index2VirtReg() does not consider subregs either.


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp:119
+      for (auto &Def : MRI->def_instructions(SrcReg)) {
+        if (SrcSubReg != Def.getOperand(0).getSubReg())
+          continue;
----------------
rampitec wrote:
> I am no sure why do you need to match SRC and DST subregs. Why not just copy a source from Def here whatever it is?
Same as other comment. There may be subregs that def_instructions does not account for. We need to check if we are copying the correct subreg.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108830