[PATCH] D88060: [GISel]: Few InsertVecElt combines

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added inline comments.


================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:521
+
+def insert_vec_elt_combines : GICombineGroup<[combine_consecutive_insert_vec_elts,
+                                              combine_ins_vec_build_vec]>;
----------------
foad wrote:
> Line length.
Can do.


================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:551
 
-def all_combines : GICombineGroup<[trivial_combines, ptr_add_immed_chain,
+def all_combines : GICombineGroup<[trivial_combines, insert_vec_elt_combines, ptr_add_immed_chain,
     combines_for_extload, combine_indexed_load_store, undef_combines,
----------------
foad wrote:
> Line length.
Can do.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:2462
+  // If we didn't end in a G_IMPLICIT_DEF, bail out.
+  return TmpInst->getOpcode() == TargetOpcode::G_IMPLICIT_DEF;
+}
----------------
foad wrote:
> It seems like it would be very easy to handle G_BUILD_VECTOR here as well as G_IMPLICIT_DEF. Then it will handle a chain of G_INSERT_VECTOR_ELT ending in a G_BUILD_VECTOR, with no need for a separate match function. Win-win?
While it's easy to do, I think it's important to not cram too many combines into one combine. IIUC, the goal with the current table gen wrapper/split is for us to be able to declare/express the same combines/matching ability with the new Combiner table gen framework. So conceptually we probably want to keep them separate for more match function/expressibility test cases for the new combiner (I guess how to separate is quite arbitrary at this point).
TLDR; I can certainly update this, but do we want to cram many peephole combines into one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88060