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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh 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:
> 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.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp:166-170
+  // 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;
----------------
arsenm wrote:
> Don't see why this would restrict the vector type
Shouldn't this just be on 2-element vectors?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135145