[LLVM] Optimize G_SHUFFLE_VECTOR into more efficient generic opcodes

[ValentijnvdBeek]

This merge requests implements an infrastructure based on streams that allows for the creation of shuffle vector pattern masks that can be used to replace inefficient scalarizations with faster generic opcodes. It implements five patterns that are useful for the AIE going forwards, namely one that matches the concatenation of vectors, the same in reverse, the insertion of a vector and the merging of two vectors.

Some todos in the merge request that can be done after the rest has been OK'ed

• The assertions that check the initial state are done a bit adhoc, it might be nice to pass them as function pointers as well
• Inefficient matching for larger groups of patterns. The patterns now match the minimum of what they can. For example, only the first two parts of vectors are transformed. Most of these can be extended, but this leads to very large set of patterns quickly
• There are two matching functions, since the concat version does a lot of additional work that I am not sure what it does. I should probably figure out if those can be fully merged or whether just the simply variant ought to survive
• Use llvm::SmallVector and not std::vector

[konstantinschwarz]

Yay!

[konstantinschwarz]

🚀

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff ea89ed6223aa93251b9708c16b9d605ab30dccdb 07244e73f9104198bb9cb52273afc033367e0429 -- llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp llvm/lib/Target/AIE/AIE2PreLegalizerCombiner.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
index 70297e7d43..b42b0b7c29 100644
--- a/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
@@ -352,32 +352,31 @@ Register CombinerHelper::createUnmergeValue(
     TargetReg = MRI.createGenericVirtualRegister(HalfSizeTy);
   }
 
-    // Each destination fits n times into the source and each iteration we
-    // exactly half the source. Therefore we need to pick on which side we want
-    // to iterate on.
-    const uint32_t DstNumElements =
-        DstTy.isVector() ? DstTy.getNumElements() : 1;
-    const uint32_t HalfWay = Start + ((End - Start) / 2);
-    const uint32_t Position = DestinationIndex * DstNumElements;
-
-    uint32_t NextStart, NextEnd;
-    if (Position < HalfWay) {
-      Builder.buildInstr(TargetOpcode::G_UNMERGE_VALUES, {TargetReg, TmpReg},
-                         {SrcReg});
-      NextStart = Start;
-      NextEnd = HalfWay;
-    } else {
-      Builder.buildInstr(TargetOpcode::G_UNMERGE_VALUES, {TmpReg, TargetReg},
-                         {SrcReg});
-      NextStart = HalfWay;
-      NextEnd = End;
-    }
+  // Each destination fits n times into the source and each iteration we
+  // exactly half the source. Therefore we need to pick on which side we want
+  // to iterate on.
+  const uint32_t DstNumElements = DstTy.isVector() ? DstTy.getNumElements() : 1;
+  const uint32_t HalfWay = Start + ((End - Start) / 2);
+  const uint32_t Position = DestinationIndex * DstNumElements;
+
+  uint32_t NextStart, NextEnd;
+  if (Position < HalfWay) {
+    Builder.buildInstr(TargetOpcode::G_UNMERGE_VALUES, {TargetReg, TmpReg},
+                       {SrcReg});
+    NextStart = Start;
+    NextEnd = HalfWay;
+  } else {
+    Builder.buildInstr(TargetOpcode::G_UNMERGE_VALUES, {TmpReg, TargetReg},
+                       {SrcReg});
+    NextStart = HalfWay;
+    NextEnd = End;
+  }
 
-    if (HalfSizeTy.isVector() && DstTy != HalfSizeTy)
-      return createUnmergeValue(MI, TargetReg, DstReg, DestinationIndex,
-                                NextStart, NextEnd);
+  if (HalfSizeTy.isVector() && DstTy != HalfSizeTy)
+    return createUnmergeValue(MI, TargetReg, DstReg, DestinationIndex,
+                              NextStart, NextEnd);
 
-    return DstReg;
+  return DstReg;
 }
 
 bool CombinerHelper::tryCombineShuffleVector(MachineInstr &MI) {

[konstantinschwarz]

There are plenty of upstream tests failing. Could you please take a look?

[gbossu]

Curious: Maybe IRTranslator already has a similar piece of code that we can re-use?

[ValentijnvdBeek]

Not from what I can see, G_UNMERGE_VALUES isn't meant for vectors to begin with. It only supports scalars, according to the documentation. Most backends just support this as an extension since there is no other opcode to do it. In theory, this will be ripped out in favour of a simple G_SUBREGISTER_EXTRACT in the near future if that opcode gets proper support. Utils.cpp does have something that is a bit similar, called llvm::extractParts, but that splits into two parts while this code goes through it like a binary tree.

In any case, I would keep it like this since this code should be replaced before upstreaming and that is easier if we are the only ones depending on it.

[ValentijnvdBeek]

G_UNMERGE_VALUE isn't really meant to be used for vectors like this, it is actually defined as a scalar opcode. There is a nice drop-in replacement opcode called G_EXTRACT_SUBVECTOR which has landed in our tree in early August, but there is no support in other backends for this yet. So this implementation is a necessary evil until the other opcode is able to replace it.

[ValentijnvdBeek]

This test is:

define <8 x i8> @i8_off8(<16 x i8> %arg1, <16 x i8> %arg2) {
entry:
  %shuffle = shufflevector <16 x i8> %arg1, <16 x i8> %arg2, <8 x i32> <i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15>
  ret <8 x i8> %shuffle
}

This turns into:

bb.1.entry:
  liveins: $q0, $q1
  %0:_(<16 x s8>) = COPY $q0
  %3:_(<8 x s8>), %2:_(<8 x s8>) = G_UNMERGE_VALUES %0:_(<16 x s8>)
  $d0 = COPY %2:_(<8 x s8>)
  RET_ReallyLR implicit $d0

This is expected

[ValentijnvdBeek]

This is:

define <8 x i8> @i8_zero_off8(<16 x i8> %arg1) {
entry:
  %shuffle = shufflevector <16 x i8> %arg1, <16 x i8> zeroinitializer, <8 x i32> <i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15>
  ret <8 x i8> %shuffle
}

And turns into:

bb.1.entry:
  liveins: $q0, $q1
  %0:_(<16 x s8>) = COPY $q0
  %5:_(<8 x s8>), %4:_(<8 x s8>) = G_UNMERGE_VALUES %0:_(<16 x s8>)
  $d0 = COPY %4:_(<8 x s8>)
  RET_ReallyLR implicit $d0

this is expected

[ValentijnvdBeek]

this is for me the most surprising test result and hardest to parse. The input LLVM IR looks like:

define i32 @add_pair_v8i16_v4i32_double_sext_zext_shuffle(<8 x i16> %ax, <8 x i16> %ay, <8 x i16> %bx, <8 x i16> %by) {
entry:
  %axx = zext <8 x i16> %ax to <8 x i32>
  %s1h = shufflevector <8 x i32> %axx, <8 x i32> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
  %s1l = shufflevector <8 x i32> %axx, <8 x i32> undef, <4 x i32> <i32 4, i32 5, i32 6, i32 7>
  %axs = add <4 x i32> %s1h, %s1l
  %ayy = zext <8 x i16> %ay to <8 x i32>
  %s2h = shufflevector <8 x i32> %ayy, <8 x i32> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
  %s2l = shufflevector <8 x i32> %ayy, <8 x i32> undef, <4 x i32> <i32 4, i32 5, i32 6, i32 7>
  %ays = add <4 x i32> %s2h, %s2l
  %az = add <4 x i32> %axs, %ays
  %bxx = zext <8 x i16> %bx to <8 x i32>
  %s3h = shufflevector <8 x i32> %bxx, <8 x i32> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
  %s3l = shufflevector <8 x i32> %bxx, <8 x i32> undef, <4 x i32> <i32 4, i32 5, i32 6, i32 7>
  %bxs = add <4 x i32> %s3h, %s3l
  %byy = zext <8 x i16> %by to <8 x i32>
  %s4h = shufflevector <8 x i32> %byy, <8 x i32> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
  %s4l = shufflevector <8 x i32> %byy, <8 x i32> undef, <4 x i32> <i32 4, i32 5, i32 6, i32 7>
  %bys = add <4 x i32> %s4h, %s4l
  %bz = add <4 x i32> %bxs, %bys
  %z = add <4 x i32> %az, %bz
  %z2 = call i32 @llvm.vector.reduce.add.v4i32(<4 x i32> %z)
  ret i32 %z2
}

This is translated into the following global isel IR:

bb.1.entry:                                                                                                                                                                                                                                                        liveins: $q0, $q1, $q2, $q3
  %0:_(<8 x s16>) = COPY $q0
  %1:_(<8 x s16>) = COPY $q1
  %2:_(<8 x s16>) = COPY $q2
  %3:_(<8 x s16>) = COPY $q3
  %33:_(<4 x s16>), %34:_(<4 x s16>) = G_UNMERGE_VALUES %0:_(<8 x s16>)
  %35:_(<4 x s32>) = G_ZEXT %33:_(<4 x s16>)
  %36:_(<4 x s32>) = G_ZEXT %34:_(<4 x s16>)
  %8:_(<4 x s32>) = G_ADD %35:_, %36:_
  %37:_(<4 x s16>), %38:_(<4 x s16>) = G_UNMERGE_VALUES %1:_(<8 x s16>)
  %39:_(<4 x s32>) = G_ZEXT %37:_(<4 x s16>)
  %40:_(<4 x s32>) = G_ZEXT %38:_(<4 x s16>)
  %12:_(<4 x s32>) = G_ADD %39:_, %40:_
  %13:_(<4 x s32>) = G_ADD %8:_, %12:_
  %41:_(<4 x s16>), %42:_(<4 x s16>) = G_UNMERGE_VALUES %2:_(<8 x s16>)
  %43:_(<4 x s32>) = G_ZEXT %41:_(<4 x s16>)
  %44:_(<4 x s32>) = G_ZEXT %42:_(<4 x s16>)
  %17:_(<4 x s32>) = G_ADD %43:_, %44:_
  %45:_(<4 x s16>), %46:_(<4 x s16>) = G_UNMERGE_VALUES %3:_(<8 x s16>)
  %47:_(<4 x s32>) = G_ZEXT %45:_(<4 x s16>)
  %48:_(<4 x s32>) = G_ZEXT %46:_(<4 x s16>)
  %21:_(<4 x s32>) = G_ADD %47:_, %48:_
  %22:_(<4 x s32>) = G_ADD %17:_, %21:_
  %23:_(<4 x s32>) = G_ADD %13:_, %22:_
  %24:_(s32) = G_VECREDUCE_ADD %23:_(<4 x s32>)
  $w0 = COPY %24:_(s32)
  RET_ReallyLR implicit $w0
                                                                                                                                               

This corresponds to the the shufflevectors perfectly and actually shows that they simplify the unmerges as well, which is a welcome bonus.

[ValentijnvdBeek]

Maybe do these tests in a callback? Now, if one fails, it will exit the combiner completely rather than continuing on. For now, this is fine, but with more patterns that are close, this might be problems.

[ValentijnvdBeek]

The input LLVM IR is:

──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────define <16 x i8> @test_concat_v16i8_v8i8_v16i8(<8 x i8> %x, <16 x i8> %y) #0 {
entry:
  %vecext = extractelement <8 x i8> %x, i32 0
  %vecinit = insertelement <16 x i8> undef, i8 %vecext, i32 0
  %vecext1 = extractelement <8 x i8> %x, i32 1
  %vecinit2 = insertelement <16 x i8> %vecinit, i8 %vecext1, i32 1
  %vecext3 = extractelement <8 x i8> %x, i32 2
  %vecinit4 = insertelement <16 x i8> %vecinit2, i8 %vecext3, i32 2
  %vecext5 = extractelement <8 x i8> %x, i32 3
  %vecinit6 = insertelement <16 x i8> %vecinit4, i8 %vecext5, i32 3
  %vecext7 = extractelement <8 x i8> %x, i32 4
  %vecinit8 = insertelement <16 x i8> %vecinit6, i8 %vecext7, i32 4
  %vecext9 = extractelement <8 x i8> %x, i32 5
  %vecinit10 = insertelement <16 x i8> %vecinit8, i8 %vecext9, i32 5
  %vecext11 = extractelement <8 x i8> %x, i32 6
  %vecinit12 = insertelement <16 x i8> %vecinit10, i8 %vecext11, i32 6
  %vecext13 = extractelement <8 x i8> %x, i32 7
  %vecinit14 = insertelement <16 x i8> %vecinit12, i8 %vecext13, i32 7
  %vecinit30 = shufflevector <16 x i8> %vecinit14, <16 x i8> %y, <16 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 16, i32 17, i32 18, i32 19, i32 20, i32 21, i32 22, i32 2>
  ret <16 x i8> %vecinit30
}

Our outputted post-combiner IR is:

bb.1.entry:
  liveins: $d0, $q1
  %0:_(<8 x s8>) = COPY $d0
  %1:_(<16 x s8>) = COPY $q1
  %3:_(s64) = G_CONSTANT i64 0
  %2:_(s8) = G_EXTRACT_VECTOR_ELT %0:_(<8 x s8>), %3:_(s64)
  %7:_(s64) = G_CONSTANT i64 1
  %6:_(s8) = G_EXTRACT_VECTOR_ELT %0:_(<8 x s8>), %7:_(s64)
  %10:_(s64) = G_CONSTANT i64 2
  %9:_(s8) = G_EXTRACT_VECTOR_ELT %0:_(<8 x s8>), %10:_(s64)
  %13:_(s64) = G_CONSTANT i64 3
  %12:_(s8) = G_EXTRACT_VECTOR_ELT %0:_(<8 x s8>), %13:_(s64)
  %16:_(s64) = G_CONSTANT i64 4
  %15:_(s8) = G_EXTRACT_VECTOR_ELT %0:_(<8 x s8>), %16:_(s64)
  %19:_(s64) = G_CONSTANT i64 5
  %18:_(s8) = G_EXTRACT_VECTOR_ELT %0:_(<8 x s8>), %19:_(s64)
  %22:_(s64) = G_CONSTANT i64 6
  %21:_(s8) = G_EXTRACT_VECTOR_ELT %0:_(<8 x s8>), %22:_(s64)
  %25:_(s64) = G_CONSTANT i64 7
  %24:_(s8) = G_EXTRACT_VECTOR_ELT %0:_(<8 x s8>), %25:_(s64)
  %34:_(<8 x s8>) = G_BUILD_VECTOR %2:_(s8), %6:_(s8), %9:_(s8), %12:_(s8), %15:_(s8), %18:_(s8), %21:_(s8), %24:_(s8)
  %31:_(<8 x s8>), %32:_(<8 x s8>) = G_UNMERGE_VALUES %1:_(<16 x s8>)
  %27:_(<16 x s8>) = G_CONCAT_VECTORS %34:_(<8 x s8>), %31:_(<8 x s8>)
  $q0 = COPY %27:_(<16 x s8>)
  RET_ReallyLR implicit $q0

The IR is alright, it correctly puts the vector together, however there is a combiner missing that takes the extractions, turns it into a shufflevector and then optimizes it out.

[ValentijnvdBeek]

Same as above

[ValentijnvdBeek]

To explain the lateness of the reviews: ASUS managed to lose my hawkpoint laptop and have been promising that they will ship it soon (tm) for the past three weeks. I will see if I can get some progress done on my old 4 core

[ValentijnvdBeek]

@konstantinschwarz @gbossu friendly review bump. Hopefully the tests don't fail (I forgot where the git-clang-format hook was again) :)

Changes from the last time:

• Split some changes up into different commits
• Fixed a few nits (the ones recently in a nit commit as I am bettering my life)
• Explained the assembly changes outside AIE2
• Rebased onto current Main

It is a bit late, so I hope I haven't missed anything. Good luck today!

[ValentijnvdBeek]

FYI: there will be a conflict issue whenever LLVM main is merged into this branch due to CombinerHelper::tryCombineShuffleVector being split up into two parts. To incorporate it into this branch, you simply put the stream matching code in the new matchVectorMaskSequence function and it should be fine.

The relevant PR is: llvm/llvm-project#110545

@jsetoain the above PR is interesting for you since it sidesteps the need to convert into shufflevector explicitly. It will optimize sequences of inserts/extracts/buildvector automatically. You may want to cherry-pick the original version which turns into a G_SHUFFLE_VECTOR and then optimizes that rather than the current one that runs the optimizer directly.

Cheers!

[konstantinschwarz]

Hi @ValentijnvdBeek, I finally came back to this, sorry for the long delay.
In #224 I implemented an alternative combine for matching the G_SHUFFLE_VECTOR -> G_EXTRACT_SUBVECTOR (still G_UNMERGE_VALUES until the next upstream sync).

It uses the already available helper functions for creating contiguous shuffle masks. WDYT?

[ValentijnvdBeek]

Hi @ValentijnvdBeek, I finally came back to this, sorry for the long delay. In #224 I implemented an alternative combine for matching the G_SHUFFLE_VECTOR -> G_EXTRACT_SUBVECTOR (still G_UNMERGE_VALUES until the next upstream sync).

It uses the already available helper functions for creating contiguous shuffle masks. WDYT?

It is fine, I am just happy if the work doesn't totally bit rot. I like the linked code, for the most part, it takes my approach and makes it less of a tech demo (which this sort of is a bit). You do cover a part of the approach though, which admittedly the one that this implements, but there are some worries that I have about the general cost of this combiner in the future. It is pretty expensive and putting it in many different combiners does add a lot of code (and causes it to check the same values a lot). But as an approach within AIE, that is probably fine.

So as of this PR, what could be an idea is that I close it and you guys pick it over for those combiners. I will then take it back, be inspired by your approach and rewrite it with some of my crazier ideas to reduce the complexity so that I can push it upstream directly. Together with maybe #129, that way it is also out of your hair and review todo list. Is that an idea @konstantinschwarz?