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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp:119
+      for (auto &Def : MRI->def_instructions(SrcReg)) {
+        if (SrcSubReg != Def.getOperand(0).getSubReg())
+          continue;
----------------
vangthao wrote:
> 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.
I see.


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp:135
+            LIS->removeInterval(SrcReg);
+            LIS->createAndComputeVirtRegInterval(DefSrcMO.getReg());
+            LIS->createAndComputeVirtRegInterval(SrcReg);
----------------
Actually you can defer LIS update until after the loop. If you are dealing with many subregs of the same register you would only need to recreate interval once.


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp:141
+          // Found the defining accvgpr_write, stop looking any further.
+          break;
+        }
----------------
rampitec wrote:
> You can just return false here.
In fact not, you still want to continue outer loop.


================
Comment at: llvm/lib/Target/AMDGPU/GCNPreRAOptimizations.cpp:144
+      }
       break;
+    }
----------------
rampitec wrote:
> ... and here.
Same thing, it should continue outer loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108830