[AIEX] Concat/Unmerge PHI Combiner #500
Conversation
F-Stuckmann
requested review from
abhinay-anubola,
abnikant,
andcarminati,
gbossu,
katerynamuts,
khallouh,
konstantinschwarz,
martien-de-jong,
niwinanto,
SagarMaheshwari99 and
stephenneuendorffer
as code owners
| combine_add_vector_elt_undef ]> { | ||
| combine_add_vector_elt_undef, | ||
| combine_concat_unmerge_phis | ||
| ]> { |
There was a problem hiding this comment.
Why are we using post-legalizer and not pre-legalizer? We are touching types here, so we can generate not legal types as well.
There was a problem hiding this comment.
the only instruction we generate is a concat in a dominating MBB, that we know is already legal.
There was a problem hiding this comment.
We also generate a phi, whose legality could be not the same as concat. This type of optimization always fits better in pre-legalization.
| /// used [ 0 (7, 9) or 1 (8, 10)]. Start the pattern checking from \p ConcatI . | ||
| /// Follow \p UseIdx (0 or 1) of G_CONCAT to identify the pattern. Collect all | ||
| /// relevant Results in \p MatchData . | ||
| static std::pair<MachineInstr *, unsigned> |
There was a problem hiding this comment.
Can we embed this return info into AIEConcatUnmergeCombineMatchData? In this way we can concentrate all information together.
| const Register Reg = ConcatI.getOperand(OperandIdx).getReg(); | ||
| auto *PhiInst = MRI.getVRegDef(Reg); | ||
| if (!PhiInst || !PhiInst->isPHI()) | ||
| return {nullptr, /*UnmergeSubVecDefIdx=*/0}; |
There was a problem hiding this comment.
Could we use optional here?
| auto [UnMergeInst, UnMergeDefMO] = FindPhiUnMergeComponents(PhiInst); | ||
|
|
||
| if (!UnMergeInst || | ||
| UnMergeInst->getOpcode() != TargetOpcode::G_UNMERGE_VALUES || |
There was a problem hiding this comment.
Unmerge can have more then 2 defs, we should check this too.
| << "\n"); | ||
|
|
||
| auto UnMergeOrigin0 = | ||
| findUnmergeOrigin(ConcatI, /*UseIdx=*/0, MRI, Helper, MatchInfo); |
There was a problem hiding this comment.
I was thinking if we can find indexes 0 and 1 in a single sweep, storing relevant data in MatchInfo.
|
|
||
| if (MatchData.ConcatMBB && MatchData.ConcatMBB != MBB.getMBB()) { | ||
| LLVM_DEBUG(dbgs() | ||
| << "Both Parts of the Incoming Concat Values do not " |
|
|
||
| // If Unmerge Origins diverge, skip | ||
| if (UnMergeOrigin0.first != UnMergeOrigin1.first) | ||
| return false; |
There was a problem hiding this comment.
CHECK: AFAIK, G_CONCAT can also take more than two operands
| @@ -30,6 +30,23 @@ struct FrequentIndexResult { | |||
| unsigned NonMatchingCount; | |||
| }; | |||
|
|
|||
| struct AIEConcatUnmergeCombineMatchData { | |||
| // New Concat Instruction | |||
There was a problem hiding this comment.
Concat instruction's MBB and rewrap?
| << " created " << *NewConcat.getInstr()); | ||
|
|
||
| // set Insertion Point to top of phi-MBB | ||
| B.setInsertPt(*ConcatI.getParent(), ConcatI.getParent()->begin()); |
There was a problem hiding this comment.
nit: funny that we use three different methods to set the insertion point. I guess the earlier ones are specializations of this one?
| // Concat Components | ||
| MatchData.ConcatMBB = MBB.getMBB(); | ||
| if (UseIdx == 0) | ||
| MatchData.ConcatSub0 = IncomingMO; |
There was a problem hiding this comment.
Looks as if ConcatSub could be an array of two elements
| return false; | ||
|
|
||
| // If Concat behaves like a vector broadcast, i.e. Concat uses the same | ||
| // Sub-vector in both source operands, skip |
There was a problem hiding this comment.
The (local) essence is that we only have one phi node, so the pattern doesn't match. If we want to leave this pattern to be canonicalized to broadcast and then be further optimized, I would say that we widen the combine to handle all CONCAT patterns and rewrite to broadcast here.
Alternatively, we recognize that rewriting CONCAT to broadcast should have a higher priority and we shouldn't expect occurrences here.
| AIEConcatUnmergeCombineMatchData &MatchData) { | ||
| auto *FirstMI = &*ConcatI.getParent()->begin(); | ||
|
|
||
| auto FindPhiUnMergeComponents = [UseIdx, &FirstMI, &Helper, &MRI, |
There was a problem hiding this comment.
I think it makes sense to specify the return type explicitly. Note that it also saves typing the dummy return value's type.
| findUnmergeOrigin(MachineInstr &ConcatI, unsigned UseIdx, | ||
| MachineRegisterInfo &MRI, CombinerHelper &Helper, | ||
| AIEConcatUnmergeCombineMatchData &MatchData) { | ||
| auto *FirstMI = &*ConcatI.getParent()->begin(); |
There was a problem hiding this comment.
nit: avoid use of "auto" they are definitely useful for coding, but decreases readability of code significantly.
Added a Concat/Unmerge PHI Combiner.
QoR does not show any results, because the kernels, that use this pattern are currently failing.
However, in ReduceL2C8Axis_1_aie2_bf16 the II can be reduced from 9 to 8.