HomePhabricator

[LV] Set up branch from middle block earlier.

Authored by fhahn on Dec 27 2020, 7:13 AM.

Description

[LV] Set up branch from middle block earlier.

Previously the branch from the middle block to the scalar preheader & exit
was being set-up at the end of skeleton creation in completeLoopSkeleton.
Inserting SCEV or runtime checks may result in LCSSA phis being created,
if they are required. Adjusting branches afterwards may break those
PHIs.

To avoid this, we can instead create the branch from the middle block
to the exit after we created the middle block, so we have the final CFG
before potentially adjusting/creating PHIs.

This fixes a crash for the included test case. For the non-crashing
case, this is almost a NFC with respect to the generated code. The
only change is the order of the predecessors of the involved branch
targets.

Note an assertion was moved from LoopVersioning() to
LoopVersioning::versionLoop. Adjusting the branches means loop-simplify
form may be broken before constructing LoopVersioning. But LV only uses
LoopVersioning to annotate the loop instructions with !noalias metadata,
which does not require loop-simplify form.

This is a fix for an existing issue uncovered by D93317.

Details

Committed
fhahnDec 27 2020, 10:21 AM
Parents
rG8299fb8f2564: [Transforms] Use llvm::append_range (NFC)
Branches
Unknown
Tags
Unknown

Event Timeline

Ayal added a subscriber: Ayal.Dec 29 2020, 2:31 AM

Good catch and fix! Minor post-commit comments.

/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3317

Would it be better to keep all the logic of setting the condition in completeLoopSkeleton(), including setting it to True when desired, and have the initial skeleton introduce a conditional branch here with an invalid null condition? Also simplifies the comment.

/llvm/test/Transforms/LoopVectorize/skeleton-lcssa-crash.ll
107

nit: 'inner' but not 'innermost'; may be clearer to rename 'inner' >> 'middle', and have 'loop3' be 'inner'; or rename them all loop1, loop2, loop3.