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

[Jay Foad via Phabricator via llvm-commits]

foad accepted this revision.
foad added a comment.
This revision is now accepted and ready to land.

LGTM, thanks! Just a load of optional nits inline.



================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:437
+
+  bool applyBuildVecFromRegs(MachineInstr &MI,
+                             SmallVectorImpl<Register> &MatchInfo);
----------------
Nit: make the "match" and "apply" function names match, now that they are one-to-one?


================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:508
 
+def regs_small_vec : GIDefMatchData<"SmallVector<Register, 4>">;
+def combine_insert_vec_elts_build_vector : GICombineRule<
----------------
Nit: since you have given this a generic name, there are probably other places in this file that could make use of it instead of defining their own matchdata.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:2440
+  LLT DstTy = MRI.getType(DstReg);
+  assert(DstTy.isVector() && "Invalid G_INSERT_VECTOR_ELT?");
+  unsigned NumElts = DstTy.getNumElements();
----------------
Nit: the assert seems redundant since getNumElements will do it for you.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:2457
+      return false;
+    if (!MatchInfo[IntImm].isValid())
+      MatchInfo[IntImm] = TmpReg;
----------------
Nit: I don't think you even need the .isValid() here, do you?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:2479
+  Register UndefReg;
+  auto GetUndef = [&]() {
+    if (UndefReg.isValid())
----------------
Nit: could inline this into its only caller.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:2487
+  for (unsigned I = 0; I < MatchInfo.size(); ++I) {
+    if (!MatchInfo[I].isValid())
+      MatchInfo[I] = GetUndef();
----------------
Nit: don't need the .isValid() ?


================
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:
> > 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.
Agreed it's subjective, but for compile time reasons I prefer to simplify the implementation unless there's a really good conceptual reason to separate it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88060