[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: include/llvm/CodeGen/GlobalISel/CombinerHelper.h:108
+  void applyCombineConcatVectors(MachineInstr &MI, bool IsUndef,
+                                 const SmallVectorImpl<Register> &Ops);
+
----------------
ArrayRef?


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:92-94
+bool CombinerHelper::tryCombineConcatVectors(MachineInstr &MI) {
+  bool IsUndef = false;
+  SmallVector<Register, 4> Ops;
----------------
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.


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:104-105
+                                               SmallVectorImpl<Register> &Ops) {
+  if (MI.getOpcode() != TargetOpcode::G_CONCAT_VECTORS)
+    return false;
+  IsUndef = true;
----------------
I would expect this to be an assert


================
Comment at: test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-concat-vectors.mir:25-27
+    %4:_(<2 x s64>) = G_BUILD_VECTOR %0(s64), %1
+    %5:_(<2 x s64>) = G_BUILD_VECTOR %2(s64), %3
+    %6:_(<4 x s64>) = G_CONCAT_VECTORS %4(<2 x s64>), %5
----------------
Can you add some tests with pointer elements?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69071