[PATCH] D73940: GlobalISel: Reimplement moreElementsVectorDst

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic added a comment.

If not a new LegalizeAction maybe some other way  ( like one more argument ) for targets to chose whether to create G_EXTRACT or CONCAT_VECTORS + G_UNMERGE_VALUES ?
I am not that familiar with the problem we are dealing here, but shouldn't insert + extract that are created during moreElements combine away like anyext + trunc for widen scalar ?
So this would be `extract + insert into implicit def` -> `copy extract source` if it had same type like implicit def (this is 3 instruction combine). Might that fix some of the problems?



================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1447
+  // %3:_(<12 x s32>) = G_CONCAT_VECTORS %1, %2, %2
+  // %0:_(<3 x s32>), %4:_(<3 x s32>), %5:_(<3 x s32>) = G_UNMERGE_VALUES %3
+  if (NumMergeParts > 1) {
----------------
nit: there should be one more dead def here 
```
// %0:_(<3 x s32>), %4:_(<3 x s32>), %5:_(<3 x s32>), %6:_(<3 x s32>) = G_UNMERGE_VALUES %3
```


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-and.mir:360-361
     ; CHECK: [[DEF1:%[0-9]+]]:_(<4 x s16>) = G_IMPLICIT_DEF
-    ; CHECK: [[EXTRACT1:%[0-9]+]]:_(<3 x s16>) = G_EXTRACT [[DEF1]](<4 x s16>), 0
+    ; CHECK: [[CONCAT_VECTORS:%[0-9]+]]:_(<12 x s16>) = G_CONCAT_VECTORS [[DEF]](<4 x s16>), [[DEF1]](<4 x s16>), [[DEF1]](<4 x s16>)
+    ; CHECK: [[UV:%[0-9]+]]:_(<3 x s16>), [[UV1:%[0-9]+]]:_(<3 x s16>), [[UV2:%[0-9]+]]:_(<3 x s16>), [[UV3:%[0-9]+]]:_(<3 x s16>) = G_UNMERGE_VALUES [[CONCAT_VECTORS]](<12 x s16>)
     ; CHECK: [[DEF2:%[0-9]+]]:_(<4 x s16>) = G_IMPLICIT_DEF
----------------
> The artifact combiner already handles unmerging from a concat_vector with a mixed number of elements
In some nice cases yes, here however
```
      if (NumDefs % NumMergeRegs != 0)
        return false;
```
4 % 3 != 0

Anyway this is probably equivalent to <3xs16> G_EXTRACT from  first <4xs16> in G_CONCAT_VECTORS since unmerge created 3 more dead defs. That is what we had before this patch. This works because combiner didn't combine artifacts and they are legal.
If combiner was able to combine G_UNMERGE_VALUES + CONCAT_VECTORS it would ruin work done by moreElements. There was some discussion in some patch review that targets should also be able to chose which combines to use like they chose how to legalize. That might be useful here. 


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

https://reviews.llvm.org/D73940