[AIEX] OuterLoopPipeliner Bug fix 1 #1072
Closed
F-Stuckmann wants to merge 22 commits into
Closed
Conversation
Hoist the duplicated set.loop.iterations/start.loop.iterations intrinsic predicate into a shared AIEIRUtils helper and call it from the outer-loop pipeliner's cool-down and split-prologue paths.
Factor the repeated clone-then-remap idiom into cloneInstInto, cloneAndRemapInsts, and remapClonesInto, and adopt them in clonePrologueIntoEpilogue.
Replace the ad-hoc 'outer header == prologue' checks with a BlockRegion
wrapper and region-membership predicates (isRegionInternalPhi,
isPipelineableValue, isPipelineCandidate). Validation now flows through
discoverPrologueRegion and cloning through cloneRegionSubgraph; behaviour
stays linear-only (region == {outer.header}).
Split the ~160-line cool-down builder into named step helpers (getExitBlock, cloneHardwareLoopSetupInto, createCooldownSkeleton, cloneInnerLoopIntoCooldown, populateCooldownExit, wireCooldownIntoCFG) so the body reads as an ordered list of intentions.
Mechanical rename of the split-prologue identifiers (collectPart2Instructions, Part1Set/Part2Set, Part2Insts) to spelled-out PartOne/PartTwo forms.
Build the steady-state (N-1) loop and the peeled last iteration as fresh cloned blocks, wire them into the CFG, and delete the original nest, instead of mutating the original loop in place. The original and steady-state loop structures now own disjoint blocks, removing the prior aliasing where one LoopStructure's blocks changed meaning over the pass. Steady-state clone blocks use the steady.* naming convention. Behavior-preserving: generated assembly and pipeliner remarks are unchanged.
Rename the pipeliner's identifiers and IR block/value names to a consistent scheme: the steady-state clone uses steady.* and the peeled last iteration uses lastiter.* (replacing the prior warm-up / cool-down / cooldown terminology and .cd suffixes). C++ helpers and locals follow suit (PartOneInsts, peelLastIteration, LastIter*/Peel*). Pure renaming; generated assembly and pipeliner remarks are unchanged.
Collapse multi-paragraph explanatory blocks and trailing end-of-line comments to the one-to-two-line form, replacing prose with the code it restates.
Validate the candidate loop inside the LoopStructure(Loop *) constructor and cache the result, replacing the separate optional-returning analysis helper. Now-internal analysis methods become private.
Extract the pipelining decision into a helper where each precondition is a flat early return, leaving runOnLoop to own only the subloop recursion.
Add a shared AIEIRUtils helper for the @llvm.loop.decrement check and use it from the inner-loop hardware-loop test instead of reimplementing the intrinsic-matching boilerplate.
Captures pre-fix pipeliner output for two shapes: a latch-resident accumulator and a non-phi latch def read after the loop. Both exit values are computed from poison because the last-iteration epilogue drops the loop-carried live-outs.
Source the last-iteration epilogue from the pristine original latch and remap the outer-loop exit live-outs through the last-iteration clones, so a value accumulated in (or carried out of) the latch and read after the loop is computed correctly in the final iteration instead of from poison. Regenerates the latch-accumulate and exit-no-phi tests to corrected output.
F-Stuckmann
requested review from
abhinay-anubola,
abnikant,
andcarminati,
katerynamuts,
khallouh,
konstantinschwarz,
martien-de-jong and
mludevid
as code owners
F-Stuckmann
requested review from
SagarMaheshwari99,
niwinanto and
stephenneuendorffer
as code owners
Collaborator
|
Can we at least have a description of the bug it fixes and a pointer to where it is fixed? This is unreviewable. I think squashing makes sense both jere and in the NFC PR. |
Collaborator
Author
|
I am splitting the OLP refactor into NFC parts that reimplement the bugs and the fixes on top of the refactor. that will be clearer to review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@khallouh the first bug fix for the OuterLoopPipeliner. You can see that i only discovered it after extensive refactors.
I don't think properly porting the bug fix into nice 2 single commits is worth the effort, because the NFC refactor will clean up this intermediate state anyway again.
We may want to squash the refactors though.