[AIEX] Combine G_SHUFFLE_VECTOR to COPY #346

katerynamuts · 2025-02-11T10:32:29Z

No description provided.

martien-de-jong · 2025-02-11T11:04:53Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

+    return false;
+
+  // If the mask has only -1 (undef), do nothing
+  auto allUndefs = [&NumSrcElems](const ArrayRef<int> &Mask) -> bool {


capture by value?

martien-de-jong · 2025-02-11T11:07:29Z

llvm/test/CodeGen/AIE/GlobalISel/prelegalizercombiner-shuffle-vector.mir

+body:             |
+  bb.1:
+    ; CHECK-LABEL: name: shuffle_vector_to_copy_scalar
+    ; CHECK: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 0


nit: not really a copy

llvm/test/CodeGen/AIE/GlobalISel/prelegalizercombiner-shuffle-vector.mir

martien-de-jong · 2025-02-11T11:17:23Z

llvm/test/CodeGen/AIE/GlobalISel/prelegalizercombiner-shuffle-vector.mir

    %1:_(<16 x s32>) = COPY $x0
    %2:_(<16 x s32>) = COPY $x1
    %0:_(<16 x s32>) = G_SHUFFLE_VECTOR %1(<16 x s32>), %2(<16 x s32>), shufflemask(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15)
    PseudoRET implicit $lr, implicit %0
 ...
+# Note: currently not commutable


I think this is exactly the same as matching a sawtooth with period, amplitude and height parameters that we saw recently. May I suggest to factor that code out, and build the commutable version by checking the resulting height and select the appropriate operand?

Tip: I would create a base class that generalizes a mask match. The instances will fix free parameters, like the height in this example, while matching. A base class for a sawtooth has period, amplitude and height as free parameters. Specializations require for instance Period == amplitude. Different constructors can pre-fix specific free parameters.

A negative amplitude would indicate possibilities for shuffle modes that reverse.

martien-de-jong · 2025-02-11T11:30:19Z

llvm/test/CodeGen/AIE/GlobalISel/prelegalizercombiner-shuffle-vector.mir

+    %2:_(<2 x s32>) = COPY $l1
+    %0:_(<2 x s32>) = G_SHUFFLE_VECTOR %1(<2 x s32>), %2(<2 x s32>), shufflemask(1, 2)
+    PseudoRET implicit $lr, implicit %0
+...


Perhaps add a boundary case test, e.g. a mask with eight elements, which only match in the first seven positions

andcarminati · 2025-02-11T11:32:01Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

+    return false;
+
+  // If the mask has only -1 (undef), do nothing
+  auto allUndefs = [&NumSrcElems](const ArrayRef<int> &Mask) -> bool {


AllUndefs.

andcarminati · 2025-02-11T11:47:19Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

+                              BuildFnTy &MatchInfo) {
+  assert(MI.getOpcode() == TargetOpcode::G_SHUFFLE_VECTOR);
+
+  const Register DstReg = MI.getOperand(0).getReg();


Can we use something like this:

const auto [DstReg, DstTy, Src1Reg, Src1Ty] = MI.getFirst2RegLLTs();

?

But I guess that this could trigger some clang frontend bug....

With this I am getting the following error: " error: reference to local binding 'DstReg' declared in enclosing function 'llvm::matchShuffleToCopy'
MatchInfo = [=](MachineIRBuilder &B) { B.buildCopy(DstReg, Src1Reg); };"

and the same error for Src1Reg.

I think that this is a clang bug.

That means that you can explain every line from the C++ standard related to structural bindings? Respect.

llvm/lib/Target/AIE/AIECombinerHelper.cpp

martien-de-jong · 2025-02-12T15:00:22Z

llvm/lib/Target/AIE/AIECombinerHelper.h

+public:
+  SequentialPeriodicMaskMatch(unsigned Height, unsigned Period)
+      : BaseMaskMatch{Height, Period} {}
+};


These classes don't add much in this way. We could also have created BaseMaskMatch with the constructor used here.

Right, I removed the derived classes for now, if we need them, we can add them later.

martien-de-jong · 2025-02-12T17:03:49Z

llvm/lib/Target/AIE/AIECombinerHelper.h

+  }
+
+  unsigned Period = 0;
+  unsigned Height = 0;


I had imagined these as std::optional, indicating that they were free parameters. Then, the match function, isValidMask, would use this freedom to make it match the given mask. Matching on a mask without dont-care values, it would fix Height on the first element, the Amplitude on the second, and period when the sawtooth ended. It would return false the first time it couldn't adjust a free parameter. If you would know the period that you were looking for, you would initialize it with concrete values using a constructor, or a setter function.
I guess that for our purposes, this generalized sawtooth is a reasonable base, although we could make the base class even more generic. For instance, we could match the different transpose modes for VSHUFFLE in this form as well.

But don't pursue this idea at this point. It is already good to have it factored this far.

niwinanto · 2025-02-12T17:54:41Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

+    // Find the start value of the mask
+    if (FirstNotUndef) {
+      // Get the start value
+      const unsigned MaskStart = Mask[Idx] - Idx % Period;


I could reproduce the test fails with gcc build and the issue is here. Period can be zero and it results in divide by zero.

Oh right, thanks, @niwinanto! After cleaning the ccache, I still couldn't reproduce it.

llvm/lib/Target/AIE/AIECombinerHelper.cpp

martien-de-jong · 2025-02-18T08:47:12Z

I think I'll generalize this a bit more after it has been merged.

katerynamuts requested review from abhinay-anubola, abnikant, andcarminati, F-Stuckmann, gbossu, khallouh, konstantinschwarz, martien-de-jong, niwinanto, SagarMaheshwari99 and stephenneuendorffer as code owners February 11, 2025 10:32

katerynamuts force-pushed the kmuts.shuffle-to-copy branch from 7fab9f6 to 96cdf05 Compare February 11, 2025 10:32

martien-de-jong reviewed Feb 11, 2025

View reviewed changes

llvm/test/CodeGen/AIE/GlobalISel/prelegalizercombiner-shuffle-vector.mir Show resolved Hide resolved

martien-de-jong reviewed Feb 11, 2025

View reviewed changes

andcarminati reviewed Feb 11, 2025

View reviewed changes

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

katerynamuts force-pushed the kmuts.shuffle-to-copy branch from 96cdf05 to fc5ddfa Compare February 12, 2025 10:56

katerynamuts requested review from andcarminati and martien-de-jong February 12, 2025 10:57

katerynamuts force-pushed the kmuts.shuffle-to-copy branch 2 times, most recently from fa49412 to dca2a49 Compare February 12, 2025 14:21

martien-de-jong reviewed Feb 12, 2025

View reviewed changes

niwinanto reviewed Feb 12, 2025

View reviewed changes

katerynamuts force-pushed the kmuts.shuffle-to-copy branch from dca2a49 to 4c13e95 Compare February 13, 2025 07:18

niwinanto reviewed Feb 13, 2025

View reviewed changes

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

katerynamuts force-pushed the kmuts.shuffle-to-copy branch from 4c13e95 to 75868aa Compare February 13, 2025 09:31

katerynamuts requested a review from martien-de-jong February 13, 2025 09:33

katerynamuts force-pushed the kmuts.shuffle-to-copy branch 2 times, most recently from 18f174f to 30b27dd Compare February 14, 2025 11:22

katerynamuts mentioned this pull request Feb 14, 2025

[AIE2P] Combine G_SHUFFLE_VECTOR into Unmerge+Concat #357

Open

katerynamuts added 2 commits February 14, 2025 14:56

[AIEX] Combine G_SHUFFLE_VECTOR to COPY

57ae9f7

[AIEX] Add MaskMatch classes in AIECombineHelper

bca5de2

katerynamuts force-pushed the kmuts.shuffle-to-copy branch from 30b27dd to bca5de2 Compare February 14, 2025 15:02

martien-de-jong approved these changes Feb 18, 2025

View reviewed changes

martien-de-jong merged commit 42774ee into aie-public Feb 18, 2025
8 checks passed

martien-de-jong deleted the kmuts.shuffle-to-copy branch February 18, 2025 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIEX] Combine G_SHUFFLE_VECTOR to COPY #346

[AIEX] Combine G_SHUFFLE_VECTOR to COPY #346

katerynamuts commented Feb 11, 2025

martien-de-jong Feb 11, 2025

martien-de-jong Feb 11, 2025

martien-de-jong Feb 11, 2025

martien-de-jong Feb 11, 2025

martien-de-jong Feb 11, 2025

katerynamuts Feb 12, 2025

martien-de-jong Feb 11, 2025

andcarminati Feb 11, 2025

andcarminati Feb 11, 2025

andcarminati Feb 11, 2025

katerynamuts Feb 11, 2025

andcarminati Feb 11, 2025 •

edited

Loading

martien-de-jong Feb 11, 2025

martien-de-jong Feb 12, 2025

katerynamuts Feb 13, 2025

martien-de-jong Feb 12, 2025

martien-de-jong Feb 12, 2025

niwinanto Feb 12, 2025

katerynamuts Feb 13, 2025

martien-de-jong commented Feb 18, 2025

[AIEX] Combine G_SHUFFLE_VECTOR to COPY #346

[AIEX] Combine G_SHUFFLE_VECTOR to COPY #346

Conversation

katerynamuts commented Feb 11, 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

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

andcarminati Feb 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martien-de-jong commented Feb 18, 2025

andcarminati Feb 11, 2025 •

edited

Loading