[SLP] Added more missed optimiazation remarks
ClosedPublic

Authored by miloserd on Sep 28 2017, 9:14 AM.

Details

Summary

Added more remarks to SLP pass, in particular "missed" optimization remarks.
Also proposed several tests for new functionality.

For reference you may look at: https://reviews.llvm.org/rL302811

Diff Detail

Repository
rL LLVM
miloserd created this revision.Sep 28 2017, 9:14 AM
fhahn added a comment.Wed, Oct 25, 2:36 AM

Thanks for working on this! I left a few comments.

lib/Transforms/Vectorize/SLPVectorizer.cpp
4459 ↗(On Diff #117007)

The code in the if branch does not use Inst, so could we keep if (!Inst || Inst->getOpcode() != Opcode0) ?

4460 ↗(On Diff #117007)

What would a better error message look like? We emit it if the the opcodes do not match. Would it make sense to add the opcodes to the message?

4575 ↗(On Diff #117007)

I think we should be consistent with the messages. At the beginning you use something like Cannot SLP vectorize list

5272 ↗(On Diff #117007)

This is only used by the remark as far as I can see. I think it would be simpler to just to the cast when constructing the remark.

test/Transforms/SLPVectorizer/X86/remark_horcost.ll
1 ↗(On Diff #117007)

Please consider using the YAML format when in the test, like in rL302811

miloserd updated this revision to Diff 121447.Fri, Nov 3, 3:03 AM

Fixed code style, reworked tests to check YAML output

miloserd marked 5 inline comments as done.Fri, Nov 3, 3:06 AM
miloserd added inline comments.
lib/Transforms/Vectorize/SLPVectorizer.cpp
4459 ↗(On Diff #117007)

It should only emit this remark if Inst is not null. And now it uses Inst anyway

4460 ↗(On Diff #117007)

Rework it to emit instructions, I think it's better now

miloserd marked 4 inline comments as done.Fri, Nov 3, 3:07 AM
anemet added a comment.Fri, Nov 3, 8:55 AM

Please use the new closure API to emit remarks.

miloserd updated this revision to Diff 121491.Fri, Nov 3, 9:20 AM

Reworked with closure API

anemet added a comment.Fri, Nov 3, 9:45 AM

I will look at the rest of the patch in more detail later unless Florian beats me to it. Thanks for tackling this!

lib/Transforms/Vectorize/SLPVectorizer.cpp
4459–4462 ↗(On Diff #121491)

Put these are under the closure so that we only compute them if the remark is on.

anemet added a comment.Fri, Nov 3, 9:49 AM

Also by any chance, did you run this on some real code base? Some of these may trigger quite a bit and I want to make sure they are not on the top of the list. You can use opt-viewer/opt-stats.py to get a sense how frequently your remark is generated.

Also by any chance, did you run this on some real code base? Some of these may trigger quite a bit and I want to make sure they are not on the top of the list. You can use opt-viewer/opt-stats.py to get a sense how frequently your remark is generated.

Yep, I did, it did not produced much noise on our applications.
I've just compiled ~100 random files from LLVM, opt-stats.py says only slp-vectorizer/InequableTypes is in top 10 remarks with 3% of all remarks (that is ~2000 out of 68000). I suppose it's okay.

Also by any chance, did you run this on some real code base? Some of these may trigger quite a bit and I want to make sure they are not on the top of the list. You can use opt-viewer/opt-stats.py to get a sense how frequently your remark is generated.

Yep, I did, it did not produced much noise on our applications.
I've just compiled ~100 random files from LLVM, opt-stats.py says only slp-vectorizer/InequableTypes is in top 10 remarks with 3% of all remarks (that is ~2000 out of 68000). I suppose it's okay.

Excellent, thanks!

miloserd updated this revision to Diff 121525.Fri, Nov 3, 12:36 PM

Moved the code of one remark under the closure

miloserd marked an inline comment as done.Fri, Nov 3, 12:38 PM
anemet accepted this revision.Tue, Nov 14, 10:19 AM

This looks great with some minor nits (go ahead and commit after fixing them). Thanks for your work! And sorry about the delay.

lib/Transforms/Vectorize/SLPVectorizer.cpp
5304 ↗(On Diff #121525)

Remove this whitespace-only change.

5319 ↗(On Diff #121525)

We should probably print the threshold here too

5326–5332 ↗(On Diff #121525)

Ouch, thanks for fixing this.

This revision is now accepted and ready to land.Tue, Nov 14, 10:19 AM
miloserd updated this revision to Diff 122874.Tue, Nov 14, 10:47 AM

Fixed minor issues

Thanks for your time, anemet. I've just fixed these issues you told.
Can you please commit this patch? I haven't got r+w access.

miloserd marked 3 inline comments as done.Tue, Nov 14, 10:49 AM

Thanks for your time, anemet. I've just fixed these issues you told.
Can you please commit this patch? I haven't got r+w access.

Sure, will do after lunch.

Thanks for your time, anemet. I've just fixed these issues you told.
Can you please commit this patch? I haven't got r+w access.

Sure, will do after lunch.

This does not apply cleanly for me. There are 4 rejects in SLPVectorizer.cpp. Can you please rebase the patch?

Thanks for your time, anemet. I've just fixed these issues you told.
Can you please commit this patch? I haven't got r+w access.

Sure, will do after lunch.

This does not apply cleanly for me. There are 4 rejects in SLPVectorizer.cpp. Can you please rebase the patch?

It was actually the Windows line breaks that confused arcanist. Just testing now before commit.

I get two failures, can you please take a look?

LLVM :: Transforms/SLPVectorizer/AArch64/getelementptr.ll
LLVM :: Transforms/SLPVectorizer/AArch64/horizontal.ll
miloserd updated this revision to Diff 122989.Wed, Nov 15, 3:27 AM

Fixed tests

Sorry, I forgot to test it with all of the backends. Now changed failing tests as they did not support many remarks at once.
Though I haven't checked all the tests yet since I am still waiting for the local build to finish.

Just finished testing, seems OK with latest submitted version

This revision was automatically updated to reflect the committed changes.