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

Jay Foad via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 15 14:08:47 PDT 2020


foad added inline comments.


================
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;
+}
----------------
aditya_nandakumar wrote:
> 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?
I see it as logically a single combine, which happens to handle "undef" sources as well as real build_vectors. I can't imagine why you'd want to split it into two and have it run twice as slow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88060



More information about the llvm-commits mailing list