[PATCH] D69071: [GISel][CombinerHelper] Add concat_vectors(build_vector, build_vector) => build_vector

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:92-94
+bool CombinerHelper::tryCombineConcatVectors(MachineInstr &MI) {
+  bool IsUndef = false;
+  SmallVector<Register, 4> Ops;
----------------
qcolombet wrote:
> arsenm wrote:
> > Should there be a legality check here somewhere? A concat_vectors of build_vector (or possibly build_vector_trunc) is probably the preferred form of sub-32-bit element vectors, but not for 32 or larger.
> I think the goal of the helper is just to provide the combine functionality. Whether or not the combine should be applied is left to the caller.
> 
> What do you think?
That makes sense


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:104-105
+                                               SmallVectorImpl<Register> &Ops) {
+  if (MI.getOpcode() != TargetOpcode::G_CONCAT_VECTORS)
+    return false;
+  IsUndef = true;
----------------
qcolombet wrote:
> arsenm wrote:
> > I would expect this to be an assert
> I would have expected this too, but decided against it to match the style of the other match functions.
> Given the (arguably strange) use of the try methods in tryCombine, an assert does not seem desirable.
> 
> Happy to change it though :).
I replaced similar checks  in the LegalizationArtifactCombiner in r367376


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69071