Page MenuHomePhabricator

[SLP] Limit compilation time of look-ahead operand reordering heuristic.
AbandonedPublic

Authored by vporpo on Jun 28 2019, 11:42 AM.

Details

Summary

This is a fix for the compilation time increase caused by D60897. It caps the exploration performed by the look-ahead heuristic.

Event Timeline

vporpo created this revision.Jun 28 2019, 11:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2019, 11:42 AM

Please try this patch @phosek . Please let us know if it fixes the compilation-time issue. Thanks!

The defaults should probably be a new cl::opt<int>, and would be good to document the defaults somehow?

Yes, I agree @lebedev.ri they should be options with some documentation describing what they do.
Let's first get a confirmation that this is the actual cause of the compilation-time increase.

I'm getting the following error with this change:

ld.lld: llvm-project/llvm/include/llvm/IR/User.h:170: llvm::Value *llvm::User::getOperand(unsigned int) const: Assertion `i < NumUserOperands && "getOperand() out of range!"' failed.
vporpo updated this revision to Diff 207143.Jun 28 2019, 2:10 PM

Fixed the failure.

Oops sorry about that @phosek . I wonder how none of the lit tests failed. Please try this one.

I tried the updated version and the build successfully finished and the build times are back to normal. Thank you for looking into this so promptly. Is it possible to land this change?

Thank you @phosek for testing it, I am glad it fixed the issue.
Let me add reviewers. I will push the change as soon as it gets approved by someone.

rcorcs added a subscriber: rcorcs.Jun 28 2019, 4:45 PM

Just a very minor thing.

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

Maybe it is better to call this variable Look*A*headOperandsBudget, similar to the other ones.

vporpo marked an inline comment as done.Jun 28 2019, 5:01 PM
vporpo added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
158

Yes, good point.

vporpo updated this revision to Diff 207177.Jun 28 2019, 5:03 PM

Updated option variable names.

Do you think you'll be able to land this change today or shall I revert D60897? Our bots have been broken since Thursday and we don't have any test coverage for our toolchain as a consequence.

Let's ping some of the reviewers.
@RKSimon @ABataev @dtemirbulatov any comments?

Tests?

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

Do you really need this limit?

vporpo marked an inline comment as done.Jul 1 2019, 12:01 PM

Not sure what kind of test would be suitable for this change. It simply restricts the look-ahead heuristic to not visit more than 2 operands and 2 uses, which is not something visible in the output IR.

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

Yes, because it could be a call /phi instruction with many operands.

Not sure what kind of test would be suitable for this change. It simply restricts the look-ahead heuristic to not visit more than 2 operands and 2 uses, which is not something visible in the output IR.

You still need the test that demonstrates the effect of this change. You need to show that the option really affects the vectorization somehow.

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

Does this really the cause of the problems? It would be good to see some additional investigation if this is really required.

vporpo marked an inline comment as done.Jul 1 2019, 12:29 PM

OK let me try to create a test that causes a change in the IR.

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

This can cause problems if you have a look-ahead depth > 2. Consider the case where you have chains of phis or calls. You can spend a lot of time visiting all their operands.
Though I agree, restricting the uses is more important.

ABataev added inline comments.Jul 1 2019, 12:38 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
921

I would suggest investigating if this is really required. Most probably, users lookup budget should be enough. If the second change is really required, I would suggest to prepare it as a separate patch.

Since this review is ongoing, please revert the original patch to unblock people. There is no harm in reverting and landing with the fix once it is ready. =]

We're seeing several compilation timeouts as well. Temporarily reverted in r364846 since this review is ongoing.

vporpo abandoned this revision.Jul 1 2019, 3:09 PM

Abandoning this patch. The fix can now be found in D60897.