[PATCH] D135145: [AMDGPU][GISel] Combine G_INSERT_VECTOR_ELT to G_SHUFFLE_VECTOR

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCombine.td:48-52
+def insert_vec_elt_to_shuffle : GICombineRule<
+  (defs root:$insertelt, unsigned_matchinfo:$matchinfo),
+  (match (wip_match_opcode G_INSERT_VECTOR_ELT):$insertelt,
+      [{ return PreLegalizerHelper.matchInsertVectorEltToShuffle(*${insertelt}, ${matchinfo}); }]),
+  (apply [{ PreLegalizerHelper.applyInsertVectorEltToShuffle(*${insertelt}, ${matchinfo}); }])>;
----------------
arsenm wrote:
> Pierre-vh wrote:
> > arsenm wrote:
> > > This is generic
> > Do you mean it should go in the generic combiner?
> > I'm worried that if we put it there, and remove the filtering on small vectors/hasScalarPackInsts that all INSERT_VECTOR_ELT instructions will become SHUFFLE_VECTOR and that some targets won't like it? 
> > 
> > If we remove
> > ```
> >   if (!MI.getMF()->getSubtarget<GCNSubtarget>().hasScalarPackInsts())
> >     return false;
> > 
> >   // TODO: Only on small vectors?
> >   LLT VecTy = MRI.getType(MI.getOperand(0).getReg());
> >   if (VecTy.getElementType() != LLT::scalar(16) ||
> >       (VecTy.getSizeInBits() % 32) != 0)
> >     return false;
> > ```
> > 
> > I would leave it in the AMDGPUCombiner, if we want to make it generic, I would at least add some safeguard so it doesn't turn every INSERT_VECTOR_ELT into a shuffle - maybe only do it for 2-elt vectors?
> > That or just don't add it to "all_combines" - we put it in the generic helper but it's opt-in and targets have too add the combine to their pipeline.
> Yes. This is a generic combine as it is. What the target directly wants isn't necessarily the point. A larger shuffle should be legalizable to what the target does want, and is a better canonical form 
By as-is I mean DAGCombiner 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135145