[AIE] Add combine_lag_ptr_add to AIE2PS and AIE2P#1008
Conversation
…IE2P) Adds baseline MIR tests for the lag-ptr-add pointer combine covering 1D, 2D, and 3D POSTINC_LOAD/STORE variants plus a coalesce test for both AIE2PS and AIE2P targets. The coalesce test has three run lines: - Stop after the custom combiner: shows the G_PTR_ADD rewrite in virtual regs. - Stop after the register coalescer: shows %pre/%post merged into the same vreg. - Stop after virtregrewriter: shows $p0 used directly, back-edge copy eliminated.
Add the lag-ptr-add pointer combine for AIE2PS and AIE2P targets. The combine rewrites a G_PTR_ADD of a loop pre-increment pointer in terms of the corresponding POSTINC instruction's post-increment def, avoiding a redundant add: - G_PTR_ADD %pre, C → COPY %post (when C == step) - G_PTR_ADD %pre, C → G_PTR_ADD %post, (C - step) (otherwise) To abstract over target-specific POSTINC operand layouts (1D, 2D, 3D) a virtual getPtrPostIncOpInfo() is added to AIEBaseInstrInfo, overridden in AIE2PInstrInfo and AIE2PSInstrInfo to return the PostPtrDef, PrePtrUse, and ModifierUse operand indices for each supported opcode.
katerynamuts
requested review from
F-Stuckmann,
SagarMaheshwari99,
abhinay-anubola,
abnikant,
andcarminati,
khallouh,
konstantinschwarz,
martien-de-jong,
mludevid,
niwinanto and
stephenneuendorffer
as code owners
niwinanto
left a comment
niwinanto
left a comment
There was a problem hiding this comment.
In general PR looks good to me, few nits. How ever I am wondering why these cases are not really covered by AIE Pointer Modifier Optimization. @F-Stuckmann any thoughts?
| continue; | ||
| // Find a POSTINC instruction with PreReg as base pointer. | ||
| auto MaybeInfo = TII.getPtrPostIncOpInfo(UseMI.getOpcode()); | ||
| if (!MaybeInfo) |
There was a problem hiding this comment.
Nit: PtrPostIncOpInfo make it more easy to understand
| for (const MachineInstr &UseMI : MRI.use_nodbg_instructions(DefReg)) { | ||
| if (UseMI.mayLoadOrStore() && !UseMI.hasUnmodeledSideEffects()) | ||
| return std::nullopt; | ||
| } |
There was a problem hiding this comment.
Do we need this block in this helper function? Can be part of the main loop in the combine where it checks useMI for other legalities.
| auto Root = getLagRootPreReg(MI, MRI); | ||
| if (!Root) | ||
| return false; | ||
| auto [PreReg, C] = *Root; |
There was a problem hiding this comment.
Nit: I feel inlining this helper is more readable, what do you think. What do you think. Would be nice to update the name of variables, it can really helps for readability.
const Register PreReg = MI.getOperand(1).getReg();
auto Step = getIConstantVRegValWithLookThrough(MI.getOperand(2).getReg(), MRI);
if (!Step )
return false;
const int64_t StepSize = Step->Value.getZExtValue();
| continue; | ||
| // Replacement offset NewC = C - step must fit in the original offset | ||
| // type; otherwise the G_CONSTANT built in apply silently wraps. | ||
| const int64_t S = MaybeStep->Value.getSExtValue(); |
There was a problem hiding this comment.
Nit: PostIncStepSize may be?
| # RUN: llc --mtriple=aie2ps \ | ||
| # RUN: --start-before=aie-postlegalizer-generic-combiner \ | ||
| # RUN: --stop-after=aie-postlegalizer-custom-combiner \ | ||
| # RUN: --aie-global-ptr-mod-opt=true \ |
There was a problem hiding this comment.
Nit: you can remove --aie-global-ptr-mod-opt=true, is enabled by default
|
This issue surfaces because What's still missing is the selection step: rather than always chaining off the given base pointer, we need logic to decide which base pointer to use (or reuse) when several candidates exist. |
3 participants
Add the lag-ptr-add pointer combine for AIE2PS and AIE2P targets.
The combine rewrites a G_PTR_ADD of a loop pre-increment pointer in
terms of the corresponding POSTINC instruction's post-increment def,
avoiding a redundant add: