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

Thu Oct 22 13:53:17 PDT 2020

aditya_nandakumar 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;
+}
----------------
foad wrote:
> 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.
In general it's possible to merge multiple combines into one c++ function and make the implementation simpler. However what's logically together is subjective.
In any case, I've updated as requested so we can get this in.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88060