[PATCH] D69288: [GISel][ArtifactCombiner] Relax the constraint to combine unmerge with concat_vectors

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet marked an inline comment as done.
qcolombet added a comment.

Hi @arsenm ,

I found a latent bug that has now more chances to trigger with the relaxations that this patch brings.
I've updated the patch to fix the bug, but I have now a bunch of "regressions" on AMDGPU test cases because the fix is too conservative.
Is this still okay to land?

\\ The Bug \\

Basically, before we rewrite the merge/unmerge pair, when a conversion operation is present, we need to make sure that the final source and destination types that will end up in the conversion are legit.
I've appended a test case that triggers the bug on the old (i.e., before this patch) code in `test/CodeGen/AMDGPU/GlobalISel/artifact-combiner-unmerge-values.mir`.
Without my fix, we could assert that we are trying to create conversions from incompatible types (e.g., sext s16 to <2 x s16>).
There is room for improvement, but I think this patch still goes in the right direction.

\\ The Fix \\
The fix I came up with is pretty conservative:
Let us consider:

  dstMerge = merge srcMerge
  srcUnmerge = convertop dstMerge
  dstUnmerge = unmerge srcUnmerge

If `dstUnmerge.isVector() != srcMerge.isVector()`, I block the transformation. The rationale being that we might end up rewriting the previous sequence in:

  dstUnmerge = convertop srcMerge

Cheers,
-Quentin



================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h:231
+      if (!DestTy.isVector())
+        return false;
 
----------------
This checks block some of the combines that were done with the old code.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D69288