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

[Jack Andersen via Phabricator via llvm-commits]

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.
----------------
jackoalan wrote:
> 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`.
I found a specific case where GlobalISel's IRTranslator emits `G_MERGE_VALUES`: Lowering a call with arguments that are wider than native machine types. I think `G_UNMERGE_VALUES`, `G_MERGE_VALUES`, `G_CONCAT_VECTORS`, `G_BUILD_VECTOR` should be excluded from this test because they are all reachable in CallLowering and therefore originate from IR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109750