[PATCH] D104355: [GlobalISel] Add a new artifact combiner for unmerge which looks through general artifact expressions.

Wed Jun 30 16:28:42 PDT 2021

arsenm added a comment.

This seems like a much more holistic solution. What we have now isn't manageable to guarantee every possible permutation of artifacts can legalize.

I still think legalizing G_INSERT/G_EXTRACT (especially for vectors) is problematic and probably shouldn't be done. I view them now as a thin abstraction over subregister indexes, and not really useful for vector legalization as they are used now. Having the hack for now is fine since it's definitely impossible to make any progress on this sort of thing all at once.

Are you actively working on removing the old artifact combines?

================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h:717
+      MachineInstr *Def = getDefIgnoringCopies(DefReg, MRI);
+      // If the def is from an instruction a single def, then simply delegate
+      // the search. For unmerge however with multiple defs, we need to compute
----------------
Confusing sentence "If the def is from an instruction a single def"

================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:4795-4797
+LegalizerHelper::moreElementsVectorShuffle(MachineInstr &MI,
+                                           unsigned int TypeIdx, LLT MoreTy) {
+  if (TypeIdx != 0)
----------------
Can this be split to a separate patch?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104355