This is an archive of the discontinued LLVM Phabricator instance.

[SLPVectorizer] Schedule bundle with different opcodes.
ClosedPublic

Authored by dtemirbulatov on Aug 9 2017, 6:45 AM.

Details

Summary

This change is part of https://reviews.llvm.org/D28907.

Following change let us schedule a bundle with different opcodes in it, for example : [ load, add, add, add ]

Diff Detail

Event Timeline

dtemirbulatov created this revision.Aug 9 2017, 6:45 AM
RKSimon edited edge metadata.Aug 9 2017, 7:31 AM

Test cases?

lib/Transforms/Vectorize/SLPVectorizer.cpp
975–976

You can avoid the extra indentation by an early-out

if (BundleMember->Inst != BundleMember->OpValue) {
  BundleMember = BundleMember->NextInBundle;
  continue;
}

You might even be able to tidily replace the while loop with a for loop it you used shorter variable names.

3441

early-out:

ScheduleData *ISD = getScheduleData(I);
if (!ISD)
  return false;

you can then remove that (void)ISD;

3460

weird indentation - clang-format ?

3483

weird indentation - clang-format ?

3495

weird indentation - clang-format ?

3571

Comments! You used "Handle def-use chain dependencies." for both cases!

dtemirbulatov added inline comments.Aug 10 2017, 4:52 AM
lib/Transforms/Vectorize/SLPVectorizer.cpp
975–976

Correct, Thanks.

3441

yes, right

3483

oh, thanks.

3571

oh, thanks

Test cases?

Well, I could add tests here, but it is a like a main flow of this algorithm and it should be tested already here.

RKSimon added inline comments.Aug 10 2017, 6:07 AM
lib/Transforms/Vectorize/SLPVectorizer.cpp
984

tidyup the comment

3713

possibly rephrase the assertion message: "scheduler and vectorizer bundle mismatch"?

update after RKSimon remarks and add test.

As discussed offline, the test cases need to be much simpler

lib/Transforms/Vectorize/SLPVectorizer.cpp
3445

Shouldn't the message be "ScheduleData not in scheduling region" ?

3676

assert message: "ScheduleData not in scheduling region" ?

dtemirbulatov updated this revision to Diff 110844.EditedAug 12 2017, 12:09 PM

Update after RKSimon's remarks and replace testcase for more appropriate.

Thanks for the new tests - a few minor remarks.

test/Transforms/SLPVectorizer/X86/schedul_bundel.ll
1 ↗(On Diff #110844)

typo - please rename the test file schedule-bundle.ll

28 ↗(On Diff #110844)

This test doesn't seem to be working?

91 ↗(On Diff #110844)

What is this constant store for?

99 ↗(On Diff #110844)

You can drop these attributes if you remove the (unnecessary) tbaa tags

dtemirbulatov added inline comments.Aug 13 2017, 9:38 PM
test/Transforms/SLPVectorizer/X86/schedul_bundel.ll
28 ↗(On Diff #110844)

yes, it is working, but the second function gives up better coverage, I am going to remove this one.

99 ↗(On Diff #110844)

ok, thanks.

correct test filename spelling.

RKSimon accepted this revision.Aug 14 2017, 3:08 AM

LGTM

This revision is now accepted and ready to land.Aug 14 2017, 3:08 AM
This revision was automatically updated to reflect the committed changes.