Page MenuHomePhabricator

LoadStoreVectorizer: support different operand orders in the add sequence match
ClosedPublic

Authored by wvoquine on Tue, Jun 8, 10:13 AM.

Details

Summary

First we refactor the code which does no wrapping add sequences
match: we need to allow different operand orders for
the key add instructions involved in the match.

Then we use the refactored code trying 4 variants of matching operands.

Originally the code relied on the fact that the matching operands
of the two last add instructions of memory index calculations
had the same LHS argument. But which operand is the same
in the two instructions is actually not essential, so now we allow
that to be any of LHS or RHS of each of the two instructions.
This increases the chances of vectorization to happen.

Diff Detail

Event Timeline

wvoquine created this revision.Tue, Jun 8, 10:13 AM
wvoquine requested review of this revision.Tue, Jun 8, 10:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jun 8, 10:13 AM
wvoquine set the repository for this revision to rG LLVM Github Monorepo.Tue, Jun 8, 10:15 AM
wvoquine updated this revision to Diff 350687.Tue, Jun 8, 12:30 PM

Fixed the code style issue.
Implemented operand order picking loop instead of a sequence of ifs.

The failing test seems to be not related to the change.

volkan added inline comments.Thu, Jun 10, 1:35 PM
llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
391

Nit: Add empty line?

393

You can rename MatchinOperand[AB] as MatchinOpIdx[AB] or MatchinOperandIdx[AB] to make it clear that this is an index.

415

You can move this into the if statement below as there is no other users and get the Value directly as below:

Value *OtherOpA = AddOpA->getOperand(MatchinOpIdxA == 0 ? 1 : 0);
420

This might be LHS or RHS based on MatchinOperand[AB], could you rename these variables?

430

No need to check other cases, you can return true directly and get rid of Safe.

526

Typo

wvoquine updated this revision to Diff 351270.Thu, Jun 10, 2:48 PM

Implemented the suggested naming and code style changes.

wvoquine marked 6 inline comments as done.Thu, Jun 10, 2:48 PM
wvoquine added inline comments.
llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
393

I like MatchingOpIdx[AB] - by the way, I missed g in Matching too, thanks!

volkan accepted this revision.Thu, Jun 10, 3:18 PM

LGTM, thanks Slava.

This revision is now accepted and ready to land.Thu, Jun 10, 3:18 PM
wvoquine edited reviewers, added: bogner; removed: JustinBorb.Thu, Jun 10, 3:18 PM
wvoquine marked an inline comment as done.EditedThu, Jun 10, 3:58 PM

The reported build failure is not related to the current change and is currently faced by other builds as well:

ld.lld: error: undefined symbol: Fortran::semantics::OmpStructureChecker::Enter(Fortran::parser::OmpClause::Full const&)

>>> referenced by semantics.cpp:83 (/var/lib/buildkite-agent/builds/llvm-project/flang/lib/Semantics/semantics.cpp:83)
This revision was landed with ongoing or failed builds.Thu, Jun 10, 4:32 PM
This revision was automatically updated to reflect the committed changes.