[AIE2P] Support all 64-bit-destination G_BUILD_VECTOR in legalizer #467
Conversation
khallouh
requested review from
abhinay-anubola,
abnikant,
andcarminati,
F-Stuckmann,
gbossu,
katerynamuts,
konstantinschwarz,
martien-de-jong,
niwinanto,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
| ; CHECK-NEXT: PseudoRET implicit $lr, implicit [[BITCAST1]](<8 x s8>) | ||
| %0:_(<32 x s8>) = COPY $wh0 | ||
| %143:_(<32 x s8>) = COPY $wh0 | ||
| %2:_(<8 x s8>) = G_SHUFFLE_VECTOR %143:_(<32 x s8>), %143:_, shufflemask(24, 25, 26, 27, 28, 29, 30, 31) |
There was a problem hiding this comment.
nit: maybe use a mask that cannot be optimized by the pre-legalizer combiner in a real scenario. For example 0, 25, 1, 27, 2, 29, 3, 31.
There was a problem hiding this comment.
Yes and we can keep both. We should be able to combine this later.
| ; CHECK-NEXT: vpush.hi.16 x0, x0, r0 | ||
| ; CHECK-NEXT: vpush.hi.16 x0, x0, r0 | ||
| ; CHECK-NEXT: vpush.hi.16 x0, x0, r0 | ||
| ; CHECK-NEXT: vextract.64 r1:r0, x0, #0, vaddsign1 |
There was a problem hiding this comment.
Super follow-up: can we combine this as a VBCST.16 + VEXTRACT.64?
There was a problem hiding this comment.
Yes, I used that pattern on purpose
|
|
||
|
|
||
| --- | ||
| name: test_build_vector_v8s8 |
There was a problem hiding this comment.
Please include legalizer tests for 16 and 32 bit element type cases.
There was a problem hiding this comment.
We should already have the one for v232 since it is already legal before my change. I already added the test for 4x16 in the end-2-end test buildvector.ll but I can add it here as well if needed.
| @@ -263,6 +263,16 @@ bool AIELegalizerHelper::legalizeG_BUILD_VECTOR(LegalizerHelper &Helper, | |||
| : MIRBuilder.buildBitcast(VecTy, Vec512Reg).getReg(0); | |||
| MIRBuilder.buildInstr(II->getGenericUnpadVectorOpcode(), {DstReg}, | |||
| {NewSrc2}); | |||
| } else if (DstVecSize == 64) { | |||
There was a problem hiding this comment.
I started to feel like we are over using vector registers, may be we could consider keep it in the scalar registers itself.
There was a problem hiding this comment.
Yes, I already tried the pack approach using shifts but there were more instructions than passing through the 512-bit vector. I don't mind either approach, both are pretty expensive without any pattern to combine.
| @@ -65,11 +65,11 @@ isValidVectorMergeUnmergeOp(const unsigned BigVectorId, | |||
| }; | |||
| } | |||
|
|
|||
| static LegalityPredicate isValidVectorAIEP(const unsigned TypeIdx) { | |||
| static LegalityPredicate isValidVectorAIE2P(const unsigned TypeIdx) { | |||
There was a problem hiding this comment.
It is used for other opcodes as well, we should add tests.
There was a problem hiding this comment.
I don't like the name actually. We were excluding 64-bits as invalid AIE2P vectors when it was just a question of missing support. For instance, we have legal vector types for all those sizes from 32-bits and upwards.
I would rather keep this as is and use another predicate for the other opcodes, or simply include the previous predicate directly since there is only one other action using that (for G_UNMERGE_VALUES)
No description provided.