[PATCH] D135145: [GISel] Combine G_INSERT_VECTOR_ELT to G_SHUFFLE_VECTOR

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:2688-2694
+  SmallVector<int, 4> ShuffleMask;
+  for (unsigned CurIdx = 0; CurIdx < NumElts; ++CurIdx) {
+    if (CurIdx == Idx)
+      ShuffleMask.push_back(NumElts);
+    else
+      ShuffleMask.push_back(CurIdx);
+  }
----------------
arsenm wrote:
> There are cases where insert_vector_elts combine to form shuffles but you don't seem to be handling those. This looks like you're just handling basic cases that can use build_vector (which is already implemented in matchCombineInsertVecElts). I'm not following what the shuffles are adding here
I thought we ultimately wanted insert_vector_elt to be lowered to shuffle_vector, as the latter is easier to handle (and is needed for mad_mix selection)? Is that not the case?
It the reason why we're lowering shuffle vectors now, no?

In any case, I'm not sure I understand the issue:
 - Is it that the combine is unnecessary? (Then why are working towards this? Why did we lower shuffle vector in the Legalizer?)
 - Is it that the combine as-is is fine, but should be handling more (like chained insert_vector_elt) ? But then, what makes it different from matchCombineInsertVecElts?

Note that, IIRC, matchCombineInsertVecElts only handles chains of insert_vector_elt. If there's a single one, it doesn't touch it. This combine is targeted towards single insert_vector_elts.



================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:2697
+  Builder.buildShuffleVector(Dst, Vec, OtherVec, ShuffleMask);
+  eraseInst(MI);
+}
----------------
arsenm wrote:
> I'm not really sure why this eraseInst helper exists
Not sure either, it's a Combiner helper. I thought it was doing some other things like notifying the observer but it really just calls MI.eraseFromParent().
I've removed this use and will propose a patch to remove it entirely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135145