[PATCH] D59626: [AMDGPU] Add MachineDCE pass after RenameIndependentSubregs

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

I see a couple of problems with the changes to DeadMachineInstructionElim.cpp in this patch:

- I get MachineVerifier problems because the LIS update does not take into consideration that the removal of a dead use can introduce disjoint live ranges (resulting in verifier complaining about "Multiple connected components in live interval".
- I get MachineVerifier problems because the LIS update does not update live ranges for physical registers. Consider an instruction like this `dead %42:regs = COPY $a1`, if that is removed and it is the last use of the physical register `$a1` then we need to shrink the live range for `$a1` , otherwise the MachineVerifier will complain about "Live segment doesn't end at a valid instruction".
- I do not think the `!MRI->reg_empty(Reg)` check takes debug info into consideration. Possibly introducing a debug invariance problem.
- It does not seem like this pass is using the LIS itself. So why do we even bother keeping it update? It is destroyed, and then it is up to the pass manager to detect if it needs to be recalculated by a later pass, right? Or do we know that it will be used by a later pass, and thereby we want to bother about preserving it (even if it isn't for the greater good of the pass itself). If it is like that, then we probably need to add some code comments to highlight that we only do it because some other pass needs it (for example in case that pass is removed in the future).
- Is it really true that we now preserve all possible analyses (both existing and future, both upstream and downstream), motivating the change from setPreservesCFG() to setPreservesAll(). I mean, the pass is pretty "destructive" as it removes instructions. You have only added (afaict incomplete) support to preserve the LIS. Notice that I'm not sure exactly what the difference between "PreservesAll" and "PreservesCFG" is, but I suppose it includes more analysis results besides LIS (and SlotIndices).

Maybe we can back out the change in DeadMachineInstructionElim.cpp until the above problems have been sorted out?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59626