Page MenuHomePhabricator

[SLP]Adjust GEP indices types when trying to build entries.
ClosedPublic

Authored by ABataev on Fri, Nov 12, 12:18 PM.

Details

Summary

Need to adjust the types of GEPs indices when building the tree
entries/operands. Otherwise some of the nodes might differ and
vectorizer is unable to correctly find them and count their cost.

Diff Detail

Event Timeline

ABataev created this revision.Fri, Nov 12, 12:18 PM
ABataev requested review of this revision.Fri, Nov 12, 12:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 12, 12:18 PM
RKSimon added inline comments.Sun, Nov 14, 8:53 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3876
SmallVector<ValueList> Operands(1, ValueList());

Add test showing different costs?

Add test showing different costs?

There is no cost difference, just the tests are crashing.

There is no cost difference, just the tests are crashing.

Ok, I see, so add such test to this commit?

There is no cost difference, just the tests are crashing.

Ok, I see, so add such test to this commit?

We already have these tests. Just before we did this adjustment during vector code emission but instead we should do it when building the tree entry. I have some other patches that depend on this one.

ABataev updated this revision to Diff 387238.Mon, Nov 15, 6:35 AM

Rebase + address comments

RKSimon added inline comments.Mon, Nov 15, 9:16 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6315–6316

auto *GEP0 = cast<GetElementPtrInst>(VL0);

6318

for (int J = 1, N = GEP0->getNumOperands(); J < N; ++J) {

6322

Value *V = Builder.CreateGEP(GEP0->getSourceElementType(), Op0, OpVecs);

ABataev updated this revision to Diff 387296.Mon, Nov 15, 9:25 AM

Address comments.

vporpo added inline comments.Mon, Nov 15, 11:43 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3881

Update comment explaining why this is done here instead of VectorizeTree() ?

ABataev updated this revision to Diff 387373.Mon, Nov 15, 1:03 PM

Address comments

vporpo accepted this revision.Mon, Nov 15, 1:30 PM

LGTM

This revision is now accepted and ready to land.Mon, Nov 15, 1:30 PM
This revision was landed with ongoing or failed builds.Tue, Nov 16, 5:46 AM
This revision was automatically updated to reflect the committed changes.

Hi, I can't build with this commit (see inline comment).

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3891

This is failing to build for me because IndexIdx isn't captured here.

ABataev added inline comments.Tue, Nov 16, 7:54 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3891
Orlando added inline comments.Tue, Nov 16, 8:00 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3891

Aha thanks, sorry for the noise.