[PATCH] D33105: [AMDGPU] Turn register pressure estimation into forward tracker

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/Target/AMDGPU/GCNRegPressure.cpp:305
+  MachineBasicBlock::const_iterator I = MI.getIterator();
+  I = skipDebugInstructionsForward(I, I->getParent()->end());
+  LiveRegs = LiveRegsCopy ? *LiveRegsCopy : getLiveRegsBefore(*I, LIS);
----------------
vpykhtin wrote:
> I think this should be the responsibility of the caller to account valid instructions, it would look unexpected that reset moves somewhere else.
Caller will always need to do it, otherwise reset will just crash. Then advance itself skips debug instructions, so they are consistent. If a caller resets tracker to the debug instruction and then just calls advance increasing the iterator it will not notice any difference. There is nothing good in letting caller to easily crash, there is no additional benefit of it besides the crash.


================
Comment at: lib/Target/AMDGPU/GCNRegPressure.h:150
+  // move to the state at the MI
+  void advance(const MachineInstr &MI) {
+    advanceBefore(MI);
----------------
vpykhtin wrote:
> we have some inconsistency here: upward tracker's "reset" moves to the point after the MI, "recede" moves to the point before the MI, but downward reset and advance moves at.
I do not think this is an inconsistency if we think about advance/recede as a two step move. We really want tracker to do two things, count pressure and get live-ins/live-outs. So reset just does the first step allowing us to collect required live set. If not that advance could just call advance one time to move at the argument instruction.


Repository:
  rL LLVM

https://reviews.llvm.org/D33105