[PATCH] D109858: [GlobalISel][AMDGPU] Add a -final-dce-legalizer flag to clean up dead code after legalization.

Thu Sep 16 10:01:26 PDT 2021

aemerson added a comment.

In D109858#3003494 <https://reviews.llvm.org/D109858#3003494>, @Petar.Avramovic wrote:

> Most common case where we end up with dead instrs in output is unmerge with dead deaf
> legalize unmerge using lower(unmerge is in inst list):
> Legalizing: %36:_(s16), %37:_(s16) = G_UNMERGE_VALUES %17:_(<2 x s16>)
> %37 is dead here, lower creates instrs and artifacts but we are now going through instrlist
> .. .. New MI: %49:_(s32) = G_BITCAST %17:_(<2 x s16>)
> .. .. New MI: %36:_(s16) = G_TRUNC %49:_(s32)
> .. .. New MI: %50:_(s32) = G_LSHR %49:_, %47:_(s32)
> .. .. New MI: %37:_(s16) = G_TRUNC %50:_(s32)
>
> Keep legalizing instrs: %50:_(s32) = G_LSHR %49:_, %47:_(s32) not dead, used by trunc, also legal and is now removed from all lists.
> Move to artifactlist legalization
> get to trunc: dead (%37), erase it, %50:_(s32) = G_LSHR %49:_, %47:_(s32) is now dead but is no longer in any list.
> We can't really rely on going through instructions in perfect order.
>
> I find  D109154 <https://reviews.llvm.org/D109154> to be extension of the way we re-insert artifacts into the artifactlist (they are in instlist at that moment) in artifact combiner (`while (!UpdatedDefs.empty())` loop ) where we go through all users of UpdatedDefs and retry combines for users that are artifacts.
> D109154 <https://reviews.llvm.org/D109154> similar thing only when instruction from some list gets deleted. It does not seem to me it would significantly affects performance and that we need dce as a flag. If it turns out to be expensive could we get dce by default and turn it off for -O0?

Why don't we just do this clean up unconditionally as a once-over pass? That way we don't have to complicate the logic any further (I'm also not a fan of calling things like `MI.uses()` just for checking dead instructions.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109858