[SLPVectorizer] Schedule bundle with different opcodes.

[dtemirbulatov]

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 ]

[RKSimon]

Test cases?

lib/Transforms/Vectorize/SLPVectorizer.cpp

970 ↗

(On Diff #110379)

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.

3439 ↗

(On Diff #110379)

early-out:

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

you can then remove that (void)ISD;

3459 ↗

(On Diff #110379)

weird indentation - clang-format ?

3482 ↗

(On Diff #110379)

weird indentation - clang-format ?

3494 ↗

(On Diff #110379)

weird indentation - clang-format ?

3570 ↗

(On Diff #110379)

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

[dtemirbulatov]

lib/Transforms/Vectorize/SLPVectorizer.cpp

970 ↗

(On Diff #110379)

Correct, Thanks.

3439 ↗

(On Diff #110379)

yes, right

3482 ↗

(On Diff #110379)

oh, thanks.

3570 ↗

(On Diff #110379)

oh, thanks

[dtemirbulatov]

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]

lib/Transforms/Vectorize/SLPVectorizer.cpp

984 ↗

(On Diff #110558)

tidyup the comment

3713 ↗

(On Diff #110558)

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

[dtemirbulatov]

update after RKSimon remarks and add test.

[RKSimon]

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

lib/Transforms/Vectorize/SLPVectorizer.cpp

3445 ↗

(On Diff #110672)

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

3676 ↗

(On Diff #110672)

assert message: "ScheduleData not in scheduling region" ?

[dtemirbulatov]

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

[RKSimon]

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]

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.

[dtemirbulatov]

update test.

[dtemirbulatov]

correct test filename spelling.

[RKSimon]

LGTM