[AIE2P] Fix combine G_SHUFFLE_VECTOR into G_AIE_VSEL #294

katerynamuts · 2025-01-21T11:14:07Z

There are some new comments in this merged PR: #272
This PR addresses all comments from the previous PR.

niwinanto · 2025-01-23T08:52:38Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

@@ -1791,32 +1791,57 @@ bool llvm::matchShuffleToVSel(

  const LLT DstTy = MRI.getType(DstReg);
  const LLT Src1Ty = MRI.getType(Src1Reg);
-  if (Src1Ty.getSizeInBits() != 512)
+  if (Src1Ty.getSizeInBits() != 512 || Src1Ty == LLT::scalar(64))


We should be checking Src1Ty.getElementType() == LLT::scalar(64), I mean we should bail out for<8 x s64>. Also, good idea to have a test.

I can see there is a test already, not sure how it works. May be mask value is wrong.

You're right, the mask was wrong. I fixed it.

niwinanto · 2025-01-23T08:54:00Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

+  int CurrIdx = Mask[I] % NumSrcElems;
+  if (CurrIdx <= PrevIdx)
+    return false;
+
+  PrevIdx = CurrIdx;
+  ++I;


I think this is not really required, for loop below should handle.

niwinanto · 2025-01-23T09:10:54Z

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

+    ; CHECK-NEXT: PseudoRET implicit $lr, implicit [[SHUF]](<8 x s64>)
+    %1:_(<8 x s64>) = COPY $x2
+    %8:_(<8 x s64>) = G_IMPLICIT_DEF
+    %0:_(<8 x s64>) = G_SHUFFLE_VECTOR %8(<8 x s64>), %1, shufflemask(8, 1, 2, 3, 11, 12, 13, 14)


mask value should be shufflemask(8, 1, 2, 3, 12, 13, 14, 15)

martien-de-jong · 2025-01-27T16:16:25Z

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

-    %4:_(<8 x s32>) = COPY $wl4
-    %3:_(<4 x s32>) = G_AIE_UNPAD_VECTOR %4(<8 x s32>)
-    %8:_(<16 x s32>) = G_AIE_PAD_VECTOR_UNDEF %3(<4 x s32>)
+    %8:_(<16 x s32>) = G_IMPLICIT_DEF
    %0:_(<16 x s32>) = G_SHUFFLE_VECTOR %8(<16 x s32>), %1, shufflemask(0, 1, 2, 3, 16, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31)


I find the name of this test strange. No index is repeated, rather, vsel can't reach index 16 in position 4.

16 is the index 0 of the second vector, that's what I meant here :) Any suggestions for the better name? Nothing comes to my mind.

I view it as element 16 not suitable for position 4. lane_mismatch? unaligned_index?

martien-de-jong · 2025-01-27T16:22:25Z

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

+    ; CHECK-NEXT: PseudoRET implicit $lr, implicit [[SHUF]](<8 x s64>)
+    %1:_(<8 x s64>) = COPY $x2
+    %8:_(<8 x s64>) = G_IMPLICIT_DEF
+    %0:_(<8 x s64>) = G_SHUFFLE_VECTOR %8(<8 x s64>), %1, shufflemask(8, 1, 2, 3, 12, 13, 14, 15)


Isn't this equivalent to {16, 17, 2, 3, 4, 5, 6, 7, 24, 25, 26, 27, 28, 29, 30, 31} on 16 x 32 ?

yes, it is but here I wanted to test that we never substitute G_SHUFFLE_VECTOR with G_AIE_VSEL for ``s64because during instruction selection we mapG_AIE_VSEL` to `VSEL` where the words can be 8, 16, or 32-bit wide.

Ok. Park it for now as an improvement story?

martien-de-jong · 2025-01-27T16:30:14Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

@@ -1810,18 +1811,37 @@ bool llvm::matchShuffleToVSel(
  // the i-th element from Src2 is used.
  // 2. The mask indices modulo the number of elements are in strictly ascending
  // order.


Could this be simplified by saying that shufflemask[i] should be -1, i or i + elemcount?
I'm assuming -1 is don't care, and we choose it to be i, leading to a zero bit in the select mask.
We then just need to run one loop, returning false as soon as we find an illegal index

andcarminati · 2025-01-29T07:33:19Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

    if (Idx >= (int)NumSrcElems) {
      unsigned long long ElemMask = 1 << I;
      DstMask |= ElemMask;
    }
  }

-  MatchInfo = std::make_tuple(DstReg, Src1Reg, Src2Reg, DstMask);
+  MatchInfo = [=, &TII](MachineIRBuilder &B) {
+    auto Cst = B.buildConstant(LLT::scalar(32), DstMask);


nit: Register MaskReg =

andcarminati · 2025-01-29T07:36:43Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

+        ++I;
+        return (Value == -1 || Value == I || Value == I + (int)NumSrcElems);
+      })) {
+    return false;
  }

  // Create the mask
  unsigned long long DstMask = 0;


I think we should replace this (and the next one) type by the standard type uint64_t.

Although unsigned long long has been longer in the C standard and is guaranteed to be at least 64 bits.

martien-de-jong · 2025-01-29T10:22:31Z

llvm/lib/Target/AIE/AIECombinerHelper.cpp

    int Idx = Mask[I];
+    if (Idx == -1)


Actually I had meant to put the full check of the above std::all_of in here; we can safely return false if the condition doesn't hold; Value == I + (int)NumSrcElems can be reused as the test to insert a one bit.
Also, the fact that we have to capture I by reference makes it a bit unnatural to use std::all_of.

if (Idx == -1 || Idx == I) continue; else if (Idx == I + NumSrcElements) DstMask |= uint64_t(1) << I; else return false;

ohh, I got it. Thanks! I changed it.

katerynamuts requested review from abhinay-anubola, abnikant, andcarminati, gbossu, khallouh, konstantinschwarz, martien-de-jong, SagarMaheshwari99 and stephenneuendorffer as code owners January 21, 2025 11:14

katerynamuts mentioned this pull request Jan 21, 2025

[AIE2P] Combine G_SHUFFLE_VECTOR into G_AIE_VSEL #272

Merged

niwinanto reviewed Jan 23, 2025

View reviewed changes

katerynamuts force-pushed the katemuts.vsel branch from e6f8920 to 8945459 Compare January 23, 2025 13:01

katerynamuts requested a review from F-Stuckmann as a code owner January 23, 2025 13:01

katerynamuts force-pushed the katemuts.vsel branch from 8945459 to 3bd5bd2 Compare January 23, 2025 13:10

katerynamuts requested a review from niwinanto January 23, 2025 13:10

martien-de-jong reviewed Jan 27, 2025

View reviewed changes

katerynamuts force-pushed the katemuts.vsel branch from 3bd5bd2 to f17984b Compare January 28, 2025 09:51

katerynamuts requested a review from martien-de-jong January 28, 2025 09:51

katerynamuts changed the title ~~[AIE2P] !Fixup Combine G_SHUFFLE_VECTOR into G_AIE_VSEL~~ [AIE2P] Fixup combine G_SHUFFLE_VECTOR into G_AIE_VSEL Jan 28, 2025

katerynamuts changed the title ~~[AIE2P] Fixup combine G_SHUFFLE_VECTOR into G_AIE_VSEL~~ [AIE2P] Fix combine G_SHUFFLE_VECTOR into G_AIE_VSEL Jan 28, 2025

katerynamuts force-pushed the katemuts.vsel branch 2 times, most recently from 8da0d8e to b95532f Compare January 29, 2025 07:19

andcarminati reviewed Jan 29, 2025

View reviewed changes

katerynamuts force-pushed the katemuts.vsel branch from b95532f to 3b5e98f Compare January 29, 2025 07:49

katerynamuts requested a review from andcarminati January 29, 2025 07:49

martien-de-jong reviewed Jan 29, 2025

View reviewed changes

[AIE2P] Fix combine G_SHUFFLE_VECTOR into G_AIE_VSEL

222db34

katerynamuts force-pushed the katemuts.vsel branch from 3b5e98f to 222db34 Compare January 29, 2025 12:20

katerynamuts requested a review from martien-de-jong January 29, 2025 12:20

martien-de-jong approved these changes Jan 29, 2025

View reviewed changes

katerynamuts enabled auto-merge (rebase) January 29, 2025 12:35

katerynamuts merged commit 0f3e26a into aie-public Jan 29, 2025
8 checks passed

katerynamuts deleted the katemuts.vsel branch January 29, 2025 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIE2P] Fix combine G_SHUFFLE_VECTOR into G_AIE_VSEL #294

[AIE2P] Fix combine G_SHUFFLE_VECTOR into G_AIE_VSEL #294

katerynamuts commented Jan 21, 2025

niwinanto Jan 23, 2025

niwinanto Jan 23, 2025

katerynamuts Jan 23, 2025 •

edited

Loading

niwinanto Jan 23, 2025

niwinanto Jan 23, 2025

martien-de-jong Jan 27, 2025

katerynamuts Jan 28, 2025

martien-de-jong Jan 29, 2025 •

edited

Loading

martien-de-jong Jan 27, 2025

katerynamuts Jan 28, 2025

martien-de-jong Jan 29, 2025

martien-de-jong Jan 27, 2025 •

edited

Loading

andcarminati Jan 29, 2025

andcarminati Jan 29, 2025

martien-de-jong Jan 29, 2025

martien-de-jong Jan 29, 2025

katerynamuts Jan 29, 2025

[AIE2P] Fix combine G_SHUFFLE_VECTOR into G_AIE_VSEL #294

[AIE2P] Fix combine G_SHUFFLE_VECTOR into G_AIE_VSEL #294

Conversation

katerynamuts commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katerynamuts Jan 23, 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

martien-de-jong Jan 29, 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

martien-de-jong Jan 27, 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

katerynamuts Jan 23, 2025 •

edited

Loading

martien-de-jong Jan 29, 2025 •

edited

Loading

martien-de-jong Jan 27, 2025 •

edited

Loading