Page MenuHomePhabricator

[LV] Fix predication for branches with matching true and false succs.
ClosedPublic

Authored by fhahn on Jan 20 2020, 10:55 PM.

Details

Summary

Currently due to the edge caching, we create wrong predicates for
branches with matching true and false successors. We will cache the
condition for the edge from the true successor, and then lookup the same
edge (src and dst are the same) for the edge to the false successor.

If both successors match, the condition should always be true. At the
moment, we cannot really create constant VPValues, but we can just
create a true condition as X | !X. Later passes will clean that up.

Fixes PR44488.

Diff Detail

Event Timeline

fhahn created this revision.Jan 20 2020, 10:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2020, 10:55 PM

Unit tests: pass. 62043 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

bjope added a subscriber: bjope.Jan 20 2020, 11:35 PM
Ayal added inline comments.Jan 22 2020, 9:59 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
6732
if (!BI->isConditional() || BI->getSuccessor(0) == BI->getSuccessor(1))
  return EdgeMaskCache[Edge] = SrcMask;

?

Best be cleaned up before LV, as it may misguide cost, but when both successors are the same the branch is essentially unconditional.

fhahn updated this revision to Diff 239656.Jan 22 2020, 11:11 AM

Updated with Ayal's suggestion.

fhahn marked an inline comment as done.Jan 22 2020, 11:15 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
6732

if (!BI->isConditional() || BI->getSuccessor(0) == BI->getSuccessor(1))

return EdgeMaskCache[Edge] = SrcMask;

?

Yes that is even simpler! I wasn't aware that we already support potentially null masks elsewhere.

Best be cleaned up before LV, as it may misguide cost, but when both successors are the same the branch is essentially unconditional.

Agreed and I don't think such branches would ever hit this point in a real-world pipeline.

Unit tests: pass. 62113 tests passed, 0 failed and 808 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Ayal accepted this revision.Jan 22 2020, 1:04 PM

Ship it!

This revision is now accepted and ready to land.Jan 22 2020, 1:04 PM
This revision was automatically updated to reflect the committed changes.