This is an archive of the discontinued LLVM Phabricator instance.

[LV] Fix for PR37248, Broadcast codegen incorrectly assumed vector loop body is single basic block
ClosedPublic

Authored by hsaito on May 1 2018, 12:10 AM.

Details

Summary

Broadcast code generation emitted instructions in pre-header, while the instruction they are dependent on in the vector loop body.
This resulted in an IL verification error ---- value used before defined.

Diff Detail

Event Timeline

hsaito created this revision.May 1 2018, 12:10 AM
Ka-Ka added a subscriber: Ka-Ka.May 2 2018, 1:28 AM
rengolin accepted this revision.May 4 2018, 1:42 AM

Nice catch! Sorry for the delay, LGTM.

This revision is now accepted and ready to land.May 4 2018, 1:42 AM
fhahn added inline comments.May 4 2018, 6:00 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
1743

IIUC we want to check here if Instr is part of the vector loop, right? Wouldn't it be more straight forward to check if the vector loop header dominates Instr?

test/Transforms/LoopVectorize/pr37248.ll
1

Not sure if the property change above means that test/Transforms/LoopVectorize/pr37248.ll is marked as executable. If that's the case, it would be great if you could remove the property before committing.

Also, is it worth adding a simple check making sure the loop is vectorized?

Renato/Florian, thanks for the review.

Ideally, we shouldn't be thinking about where to generate bcast code while we are generating vectorized code. We should have determined such a thing before generating a single line of vector code. In a long run, such a decision would be stored somewhere inside the "vectorization plan" and the code gen should just refer to that.

lib/Transforms/Vectorize/LoopVectorize.cpp
1743

Talking about straightforwardness, I suppose the most straightforward way to say it's okay to generate bcast in preheader would be "the value comes from outside of the loop". Instead of computing "Invariant" in a convoluted way, we could do

bool SafeToHoist = OrigLoop->isLoopInvariant(V) &&

(!Instr || DT->dominates(Instr->getParent(), LoopVectorPreHeader));

A lot cleaner this way?

There is one caveat. DT is still not fully up-to-date within the vectorizer's code gen
(e.g., PR37221 hits that problem). As far as the LoopVectorPreHeader and the code outside of the vectorized loop are concerned, it is up-to-date (emitMinimumIterationCountCheck() does that). That's why I think "Instr dominates preheader" check can be safely used here.

At this moment, it's best not to rely on DT w.r.t. instructions inside loop body. If not found (incorrectly), DT will return false and that'll end up in bcast code in preheader, causing a stability issue. So, using "Instruction dominates PH" is more defensive coding.

test/Transforms/LoopVectorize/pr37248.ll
1

Thanks. My bad. copy-paste error. This isn't runnable.

OK. Will check for a vector instruction.

fhahn added inline comments.May 4 2018, 11:14 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
1743

Right thanks Hideki! IMO the "SafeToHoist" variant is cleaner.

hsaito updated this revision to Diff 145425.May 6 2018, 10:38 PM

SafeToHoist logic made straightforward. FileCheck added to lit test. The test passes after the fix.

fhahn accepted this revision.May 8 2018, 1:29 AM

LGTM, thanks.

This revision was automatically updated to reflect the committed changes.