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

Tue Apr 9 08:39:19 PDT 2019

rampitec added a comment.

In D59626#1459918 <https://reviews.llvm.org/D59626#1459918>, @bjope wrote:

> In D59626#1458773 <https://reviews.llvm.org/D59626#1458773>, @rampitec wrote:
>
> > @bjope could you please check if D60419 <https://reviews.llvm.org/D60419> solves your problem with physreg use removal?
>
>
> Isn't it weird that this pass now is doing less optimizations when an analysis pass is provided? It might be OK for your use case, but it seems weird from a more general perspective.
>
> FWIW, I'm not sure exactly why we are running this pass in a lot of places in our OOT target (after ISel, before coalescing, post-coalescing, etc). But we have done it for years (since before I started working with this target), and now some of those runs won't do as much cleanup as they used to do (when a LIS is found). Not sure, but maybe it is exactly these physreg removals we aim for, so I'm not sure that solving the LIS preservation problem by not eliminating certain dead instructions is good enough for us. It will take some time to analyse that, so at the moment it is easier for me to just make a work around patch downstream.

Bjorn, thank you for the testcase. It doesn't seem to be easy to quick fix, while I am going for vacation tomorrow. I will create a revert patch for now and try to sort it out later.

I understood someone might be aiming for dead phys regs, but handling phys without recompute can be much more challenging, so probably a full recompute is not unreasonable under such scenario too.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59626