[AIE2P] Combine G_SHUFFLE_VECTOR into Extract+Broadcast #284
Conversation
katerynamuts
requested review from
abhinay-anubola,
abnikant,
andcarminati,
gbossu,
khallouh,
konstantinschwarz,
martien-de-jong,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
| @@ -1768,3 +1773,44 @@ bool llvm::matchShuffleToBroadcast(MachineInstr &MI, MachineRegisterInfo &MRI, | |||
| MatchInfo = std::make_pair(DstReg, Src1Reg); | |||
| return true; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
nit: include some gMIR example that shows the pattern to be matched.
|
|
||
| const Register Src1VecReg = MI.getOperand(1).getReg(); | ||
| const LLT ElemTy = MRI.getType(Src1VecReg).getElementType(); | ||
| if (ElemTy.getSizeInBits() == 64) |
There was a problem hiding this comment.
Do we have a reason to block this size here? For example, we are legalizing bigger types here: #283
There was a problem hiding this comment.
Agree, I removed it.
| (defs root:$root, build_fn_matchinfo:$matchinfo), | ||
| (match (wip_match_opcode G_SHUFFLE_VECTOR): $root, | ||
| [{ return matchShuffleToExtractBroadcast(*${root}, MRI, (const AIEBaseInstrInfo &)B.getTII(), ${matchinfo}); }]), | ||
| (apply [{ Helper.applyBuildFn(*${root}, ${matchinfo}); }])>; |
There was a problem hiding this comment.
Nice implementation using std::function.
| ; CHECK-NEXT: PseudoRET implicit $lr, implicit [[AIE_UNPAD_VECTOR]](<8 x s32>) | ||
| %0:_(<8 x s32>) = COPY $wl0 | ||
| %1:_(<8 x s32>) = COPY $wl1 | ||
| %2:_(<8 x s32>) = G_SHUFFLE_VECTOR %0(<8 x s32>), %1(<8 x s32>), shufflemask(2, 2, 2, 2, 2, 2, 2, 2) |
There was a problem hiding this comment.
Could you also include a negative test case?
katerynamuts
force-pushed
the
kmuts.extbcst-1
branch
2 times, most recently
from
15def52 to
296dd59
Compare
| for (unsigned I = 0; I < NumElems; I++) { | ||
| int Idx = Mask[I]; | ||
| if (Idx < 0) | ||
| continue; |
There was a problem hiding this comment.
Is that legal to have a negative index?
There was a problem hiding this comment.
yes, it means that the value is undef.
| return false; | ||
| } | ||
| } | ||
| assert(UniqOpIdx >= 0 && "Couldn't find a unique operand to extract!"); |
There was a problem hiding this comment.
Nit: To make this function more concise, I'd suggest creating a helper like: std::optional<int> getUniqueIndex(ArrayRef<int> Mask);.
katerynamuts
force-pushed
the
kmuts.extbcst-1
branch
from
296dd59 to
1fb23a9
Compare
No description provided.