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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet marked 4 inline comments as done.
qcolombet added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/CombinerHelper.h:108
+  void applyCombineConcatVectors(MachineInstr &MI, bool IsUndef,
+                                 const SmallVectorImpl<Register> &Ops);
+
----------------
arsenm wrote:
> ArrayRef?
Sure!


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


================
Comment at: lib/CodeGen/GlobalISel/CombinerHelper.cpp:104-105
+                                               SmallVectorImpl<Register> &Ops) {
+  if (MI.getOpcode() != TargetOpcode::G_CONCAT_VECTORS)
+    return false;
+  IsUndef = true;
----------------
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 :).


================
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
----------------
arsenm wrote:
> Can you add some tests with pointer elements?
Sure!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69071