[AIE2P][AIE2] Split symmetric G_BUILD_VECTOR into two G_BUILD_VECTORs #400

abhinay-anubola · 2025-03-24T09:46:54Z

Splits the symmetric G_BUILD_VECTOR into two half-sized G_BUILD_VECTOR operations and then emits a G_CONCAT_VECTORS to combine them back into the final vector.

andcarminati · 2025-03-24T10:36:39Z

llvm/lib/Target/AIE/AIECombine.td

@@ -9,6 +9,29 @@
 //===----------------------------------------------------------------------===//
 include "llvm/Target/GlobalISel/Combine.td"

+def aie_all_combines : GICombineGroup<[trivial_combines, vector_ops_combines,


Maybe we should state in a comment why we are doing this, like which combine we are excluding and why.

Could you also post some QoR numbers and if the excluded combines affect any kernels?

To be frank, I dont get why we need this change. Also, can we move this to a separate commit?

We explicit list the combines that we use, rather than getting an opaque package deal from upstream llvm. That's useful in itself. Any annotation as to why we need a particular combine would add to that value. I hope we can intersperse comment within this tablegen list?
I agree that a separate commit would be good; perhaps even two, one copying the standard list, then removing the ones we don't want. The commit comment can explain why we don't want them.

I think this solution is not scalable/maintainable, what if there are new generic combiner added? Is there no other way to filter the combiner we don't need? @konstantinschwarz ?

This feature is mentioned on llvm.org, but it is not yet available in LLVM.

def CommonPreLegalizerCombiner: GICombiner< "CommonGenPreLegalizerCombiner", [all_combines]> { let DisableRuleOption = "common-prelegalizercombiner-disable-rule"; let Modifiers = [(disable_rule copy_prop)] }

I think that listing the combiners can help to keep the generated code stable. The goal here is to exclude one combiner that undo what we are doing in this new combiner. Also, this can help to avoid surprises in case of a llvm bump.

okay, I know the goal, just wanted to check if we have a better way to do this (like what is mentioned above, but not available yet). I think we should write comment about the purpose of doing this and also a TODO- to verify the QoR after accommodating new combiner (when we bump to next version).

I don't think that <everything> - <what we explicitly don't want> is any better than <everything> in the first place. <what we don't want> is usually the result of painstakingly finding out what is bothering us, searching through an unknown bunch of combiners we have just assumed would be doing the right thing. Maintaining the list by trying newly published ones first and checking effects on QoR doesn't scare me as non-scalable.
At the point where we rebase on a new llvm, the less uncontrolled change we have, the better. Once it is up and running it is natural to play with the new toys, updating the combine list based on the release notes, or differences in the generic list.

llvm/lib/Target/AIE/AIECombinerHelper.cpp

khallouh · 2025-03-24T11:13:37Z

llvm/test/CodeGen/AIE/aie2p/buildvector.ll

+; CHECK-NEXT:    vst wl2, [p0, #0] // Delay Slot 2
+; CHECK-NEXT:    nop // Delay Slot 1
+entry:
+  store <16 x i32> <i32 111, i32 111, i32 111, i32 111, i32 111, i32 111, i32 111, i32 111, i32 222, i32 222, i32 222, i32 222, i32 222, i32 222, i32 222, i32 222>, ptr addrspace(6) null, align 32


Could we have a similar test for each size?

llvm/lib/Target/AIE/AIECombinerHelper.cpp

khallouh · 2025-03-24T11:23:02Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

+  // operand in the second half matches SecondOp.
+  for (unsigned i = 2; i <= HalfNumElts; i++) {
+    if (!MI.getOperand(i).isIdenticalTo(FirstOp)) {
+      return false;


Could we match cases when it is not identical to FirstOp but is undef?

Or if FirstOp itself is undef but all others in each half are identical...

Yep that would call for finding the 'representative' FirstOp or SecondOp on the fly. Since we will do that twice, it's probably a good idea to factor that into a function that checks each of the subranges.

llvm/lib/Target/AIE/AIECombinerHelper.cpp

martien-de-jong · 2025-03-24T12:19:15Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

+  const unsigned NumElts = MI.getNumOperands() - 1;
+  const unsigned HalfNumElts = NumElts / 2;
+  const MachineOperand FirstOp = MI.getOperand(1);
+  const MachineOperand SecondOp = MI.getOperand(HalfNumElts + 1);


I think it would be nice to have a lambda getSrcOperand(unsigned I) that captures MI, gets rid of the +1 everywhere and renormalizes the for loop below to the standard (unsigned I = 0; I < Num; I++) form.

martien-de-jong · 2025-03-24T12:47:36Z

llvm/test/CodeGen/AIE/GlobalISel/prelegalizercombiner-symmetric-build-vector.mir

+    ; CHECK-NEXT: [[AIE_BROADCAST_VECTOR:%[0-9]+]]:_(<64 x s8>) = G_AIE_BROADCAST_VECTOR [[COPY]](s32)
+    ; CHECK-NEXT: [[AIE_UNPAD_VECTOR:%[0-9]+]]:_(<32 x s8>) = G_AIE_UNPAD_VECTOR [[AIE_BROADCAST_VECTOR]](<64 x s8>)
+    ; CHECK-NEXT: [[AIE_BROADCAST_VECTOR1:%[0-9]+]]:_(<64 x s8>) = G_AIE_BROADCAST_VECTOR [[COPY1]](s32)
+    ; CHECK-NEXT: [[AIE_UNPAD_VECTOR1:%[0-9]+]]:_(<32 x s8>) = G_AIE_UNPAD_VECTOR [[AIE_BROADCAST_VECTOR1]](<64 x s8>)


CHECK: There are patterns that absorb the UNPAD_VECTORs (Ultimately we will have 32 x s8 subregs being merged to 64 x s8?)

niwinanto · 2025-03-24T13:23:51Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

+  const LLT DstVecTy = MRI.getType(DstVecReg);
+  const unsigned DstVecSize = DstVecTy.getSizeInBits();
+
+  switch (DstVecSize) {


Suggested change

switch (DstVecSize) {

if (DstVecSize < 256)

return false;

And pray no one gives us 384 bit vectors?

niwinanto · 2025-03-24T13:44:03Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

+    if (!MI.getOperand(i).isIdenticalTo(FirstOp)) {
+      return false;
+    }
+    if (!MI.getOperand(HalfNumElts + i).isIdenticalTo(SecondOp)) {


May be, no need to satisfy both half needs to be splat vectors to apply this combine, just one is enough. It helps to optimize half. Also, I can see more opportunity with some patterns, for example 3/4th with one element and 1/4th other, literately split into 2 and then into 4 giving us good optimization. What do you think?

True! if we have at least one condition, we already gain in one side.

llvm/lib/Target/AIE/AIECombinerHelper.cpp

abhinay-anubola · 2025-03-27T05:47:50Z

Working on more generic way to adding more patterns like quarters and single diff splat on parts.

martien-de-jong · 2025-04-01T13:53:59Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

+  }
+
+  // If both halves are the same register, it's effectively a splat, and the
+  // splat vector combine already handles that case.


If we have determined, at cost, that this is a splat vector, I would say there's no need to match this build vector again. Why don't we remove the splat combine, returning the splat transform lambda here?
In general, building vector patterns from subvector patterns is a great way of software reuse.

splat1 = matchSplat(0, halfsize); splat2 = matchSplat(halfsize, size); if (splat1 && splat2) { if (*splat1 == *splat2) { MatchInfo = Splat(*splat1); return true; } MatchInfo = ConcatSplat(*splat1, *splat2); return true; } return false;

We now have all the splat logic in one place; we might spot that one case is missing, which is the opportunity to implement half of this buildvector as a splat and the rest with the fall back case.

martien-de-jong · 2025-04-03T10:15:05Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

@@ -1449,7 +1445,8 @@ bool llvm::matchSymmetricBuildVector(MachineInstr &MI, MachineRegisterInfo &MRI,
        DstVecTy.changeElementCount(
            DstVecTy.getElementCount().divideCoefficientBy(2)));
  };
-  return true;
+
+  return (FirstHalfSrcReg.has_value() || SecondHalfSrcReg.has_value());


Better, but we can do more. We have now reused the matching code in the full splat vector match, but we still match twice: once to find two sub-splats and build two smaller BUILD_VECTORS and then once more to properly implement the splat vectors.
Instead, we can remove the original splat vector match and implement all splats here, because we have already done the full match. So if both subvectors are splats and identical, try to generate a full-width broadcast, otherwise if either of them are splats generate a concat with appropriate splat and buildvector.
Please note that creating a concat will hide the original full splat pattern, and we may be lucky that it is tried first, or we are painstakingly recognising the full width splat from the concat pattern.

Please leave this as a follow-up story with a TODO comment in the code.

abhinay-anubola requested review from abnikant, andcarminati, F-Stuckmann, gbossu, katerynamuts, khallouh, konstantinschwarz, martien-de-jong, niwinanto, SagarMaheshwari99 and stephenneuendorffer as code owners March 24, 2025 09:46

abhinay-anubola force-pushed the sanubola.symmetric.build.vector branch from 51b9ad9 to 6d0d5be Compare March 24, 2025 10:12

andcarminati reviewed Mar 24, 2025

View reviewed changes

khallouh reviewed Mar 24, 2025

View reviewed changes

llvm/lib/Target/AIE/AIECombinerHelper.cpp Show resolved Hide resolved

khallouh reviewed Mar 24, 2025

View reviewed changes

llvm/lib/Target/AIE/AIECombinerHelper.cpp Outdated Show resolved Hide resolved

khallouh reviewed Mar 24, 2025

View reviewed changes

llvm/lib/Target/AIE/AIECombinerHelper.cpp Outdated Show resolved Hide resolved

martien-de-jong reviewed Mar 24, 2025

View reviewed changes

llvm/lib/Target/AIE/AIECombinerHelper.cpp Outdated Show resolved Hide resolved

martien-de-jong reviewed Mar 24, 2025

View reviewed changes

niwinanto reviewed Mar 24, 2025

View reviewed changes

abhinay-anubola force-pushed the sanubola.symmetric.build.vector branch from 6d0d5be to 36549ac Compare March 26, 2025 05:55

khallouh reviewed Mar 26, 2025

View reviewed changes

llvm/lib/Target/AIE/AIECombinerHelper.cpp Outdated Show resolved Hide resolved

martien-de-jong reviewed Apr 1, 2025

View reviewed changes

abhinay-anubola added 2 commits April 3, 2025 12:08

[AIE2P][AIE2] Split symmetric G_BUILD_VECTOR into two G_BUILD_VECTORs

d8a5a0b

[AIE2][AIE2P] Allow single splat in symmetric build vector combine

1bcbfcd

abhinay-anubola force-pushed the sanubola.symmetric.build.vector branch from 36549ac to 1bcbfcd Compare April 3, 2025 06:39

martien-de-jong reviewed Apr 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIE2P][AIE2] Split symmetric G_BUILD_VECTOR into two G_BUILD_VECTORs #400

[AIE2P][AIE2] Split symmetric G_BUILD_VECTOR into two G_BUILD_VECTORs #400

abhinay-anubola commented Mar 24, 2025

andcarminati Mar 24, 2025

khallouh Mar 24, 2025

niwinanto Mar 24, 2025

martien-de-jong Mar 24, 2025

abnikant Mar 24, 2025

abhinay-anubola Mar 25, 2025 •

edited

Loading

andcarminati Mar 25, 2025

abnikant Mar 25, 2025

martien-de-jong Mar 28, 2025 •

edited

Loading

khallouh Mar 24, 2025

khallouh Mar 24, 2025

khallouh Mar 24, 2025

martien-de-jong Mar 24, 2025

martien-de-jong Mar 24, 2025

martien-de-jong Mar 24, 2025 •

edited

Loading

niwinanto Mar 24, 2025

martien-de-jong Mar 28, 2025

niwinanto Mar 24, 2025

andcarminati Mar 25, 2025

abhinay-anubola commented Mar 27, 2025

martien-de-jong Apr 1, 2025 •

edited

Loading

martien-de-jong Apr 3, 2025 •

edited

Loading

[AIE2P][AIE2] Split symmetric G_BUILD_VECTOR into two G_BUILD_VECTORs #400

Are you sure you want to change the base?

[AIE2P][AIE2] Split symmetric G_BUILD_VECTOR into two G_BUILD_VECTORs #400

Conversation

abhinay-anubola commented Mar 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinay-anubola Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martien-de-jong Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martien-de-jong Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinay-anubola commented Mar 27, 2025

martien-de-jong Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

martien-de-jong Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

abhinay-anubola Mar 25, 2025 •

edited

Loading

martien-de-jong Mar 28, 2025 •

edited

Loading

martien-de-jong Mar 24, 2025 •

edited

Loading

martien-de-jong Apr 1, 2025 •

edited

Loading

martien-de-jong Apr 3, 2025 •

edited

Loading