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

3714

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

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

28

This test doesn't seem to be working?

91

What is this constant store for?

99

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

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

99

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.