[AIE2P] Add shuffle mask range checks in matchShuffleBcstToCopy combiner

[abnikant]

If either vector source of G_SHUFFLE_VECTOR is defined by G_AIE_BROADCAST_VECTOR, combine it into a COPY operation if the mask falls within a valid range.

[andcarminati]

nit: const bool

[abnikant]

fixed.

[andcarminati]

I think this is not correct. The code was written with just one broadcast in mind. If we have two, it will get MinValue = 0 and MaxValue = (2 * NumSrcElems - 1) (full vector), and then it will select a copy from the first.

Please, test this case:

---
name:       test
alignment:       16
body:             |
  bb.1.entry:
  %1:_(s32) = COPY $r0
  %6:_(s32) = COPY $r1
  %2:_(<16 x s32>) = G_AIE_BROADCAST_VECTOR %1:_(s32)
  %3:_(<16 x s32>) = G_AIE_BROADCAST_VECTOR %6:_(s32)
  %0:_(<16 x s32>) = G_SHUFFLE_VECTOR %3:_(<16 x s32>), %2:_, shufflemask(1, 18, 18, 18, 18, 18, 18, 18, 18, 2, 18, 18, 18, 18, 18, 18)
  PseudoRET implicit $lr, implicit %0

The easiest way to fix is:

if (IsSrc1Bcst == IsSrc2Bcst)
   return false;

[martien-de-jong]

or in one go if (IsSrc1Bst + IsSrc2Bcst != 1) return false;

[abnikant]

yes, you are right. I was just thinking about one broadcast source. I have updated the code to support any or both sources as broadcast. Thanks for catching this.

[martien-de-jong]

But we could also check both ranges separately.

[martien-de-jong]

This can be further streamlined.

1. We only have to check whether it is a valid src mask if the corresponding operand is a broadcast.
2. We only have to check the second operand if the first operand fails.

if (broadcast1 && inrange(mask, 0, NumSrcElems))
  SrcReg = Src1Reg;
else if (broadcas2 && inrange(mask, NumSrcElems. NumSrcElems)
  SrcReg = Src2Reg;
else 
  return false; 

(using {start, size} convention for ranges here)

[abnikant]

refactored.

[niwinanto]

Do you think, we should take care of undef(-1) as well?

[abnikant]

do we have any practical example generating the mix of undef and def masks?

[niwinanto]

I dont know we have this pattern in mllib or not. But there were many patterns with mask containing def and undef values. Same example can be like this

/// Match something like this:
///  %1:_(s32) = COPY $r0
///  %2:_(<16 x s32>) = COPY $x0
///  %3:_(<16 x s32>) = G_AIE_BROADCAST_VECTOR %1:_(s32)
///  %0:_(<16 x s32>) = G_SHUFFLE_VECTOR %3(<16 x s32>), %2(<16 x s32>),
///  shufflemask(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, -1, -1, -1)
/// To convert to:
///  %0:_(<16 x s32>) = G_AIE_BROADCAST_VECTOR %1(s32)

[abnikant]

okay, I have updated the function to handle undef and added some tests.

[niwinanto]

nit: const bool IsValidSrc1Mask = IsSrc1Bcst ? MaskMatch(0).isMaskWithinRange(Mask, MinSrc1Value, MaxSrc1Value) : false;

[niwinanto]

Why we need llvm::any_of(Mask, [](int v) { return v != -1; })?

[abnikant]

Since I wanted it to return false when all of the masks are defined as -1

[niwinanto]

Then you could remove this check

  if (MaskMatch::isMaskWithAllUndefs(Mask))
    return false;

[abnikant]

done

[niwinanto]

LGTM!