[PATCH] D109750: [GlobalISel][Legalizer] Don't use eraseFromParentAndMarkDBGValuesForRemoval() for some artifacts.

Sun Oct 17 12:57:37 PDT 2021

jackoalan added inline comments.

================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:1054-1058
+/// These artifacts generally don't have any debug users because they don't
+/// directly originate from IR instructions, but instead usually from
+/// legalization. Avoiding checking for debug users improves compile time.
+/// Note that truncates or extends aren't included because they have IR
+/// counterparts which can have debug users after translation.
----------------
dsanders wrote:
> I don't think this assumption holds beyond the first time the legalizer processes each instruction and is dubious earlier than that if you have pre-legalization passes too.
> 
> For the standard pipeline of passes, suppose you have an expansion in the legalizer that replaces some operation with a DBG_VALUE with something ending in G_MERGE_VALUES (or one of the others). On the next legalization step you have a G_MERGE_VALUES with a DBG_VALUE. That already breaks the assumption it doesn't have one but it takes a bit more to go wrong. If anything causes this G_MERGE_VALUES to be deleted without replacement you are left with a use-without-def as the DBG_VALUE is not erased. For example, if this is one lane of a bigger G_UNMERGE_VALUES/G_MERGE_VALUES pair and something proves the lane isn't needed it would erase all the instructions for the lane but leave the DBG_VALUE behind.
> 
> I haven't dug into it myself but I'm told we've run into this scenario in our downstream target and we've had to revert this change.
I'd like to echo this claim. It is far too presumptuous to assume artifacts like `G_MERGE_VALUES` are never created pre-legalizer for all targets. If anything, this decision should be made per-target in `LegalizerInfo`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109750