Page MenuHomePhabricator

[SLP] Added more missed optimiazation remarks
AbandonedPublic

Authored by miloserd on Sep 28 2017, 12:50 AM.

Details

Reviewers
anemet
lattner
Summary

Added more remarks to SLP pass, in particular "missed" optimization remarks.
Also proposed 2 tests which I was able to easily convert from our specific tests.

For reference see: https://reviews.llvm.org/rL302811

Diff Detail

Repository
rL LLVM

Event Timeline

miloserd created this revision.Sep 28 2017, 12:50 AM
miloserd created this object with edit policy "Custom Policy".
fhahn added a comment.Sep 28 2017, 1:34 AM

Thanks for your patch! Please include the full context for the diff and add llvm-commits as subscriber; that should be done when creating the review, otherwise the mailing list does not get the complete picture. See http://llvm.org/docs/Phabricator.html for more details.

I've also added some comments about style.

lib/Transforms/Vectorize/SLPVectorizer.cpp
4450

Line too long?

4460

Line too long?

4466

AFAIK it's convention for local variable names to start with an uppercase letter.

5268

is there any reason for doing that here instead of when constructing the remark?

test/Transforms/SLPVectorizer/remark_slp_chain.c
1 ↗(On Diff #116835)

We do not want to depend on clang for LLVM unit tests (it's not part of the LLVM repo). Please convert the tests to LLVM IR test cases and use opt directly. You could use clang's -emit-llvm to start with, but ideally the test case should be as small as possible.

miloserd updated this revision to Diff 117004.Sep 28 2017, 9:00 AM

Fixed code style, reworked tests completely. Uploaded diffs with context now.

anemet edited edge metadata.Sep 28 2017, 9:03 AM

Thanks for your patch! Please include the full context for the diff and add llvm-commits as subscriber; that should be done when creating the review, otherwise the mailing list does not get the complete picture. See http://llvm.org/docs/Phabricator.html for more details.

Please start a new review cc'ing llvm-commit (from the get-go).

miloserd removed a reviewer: llvm-commits.
miloserd added a subscriber: llvm-commits.
miloserd marked 3 inline comments as done.Sep 28 2017, 9:15 AM

Thanks for your patch! Please include the full context for the diff and add llvm-commits as subscriber; that should be done when creating the review, otherwise the mailing list does not get the complete picture. See http://llvm.org/docs/Phabricator.html for more details.

Please start a new review cc'ing llvm-commit (from the get-go).

Ok, done. https://reviews.llvm.org/D38367

miloserd abandoned this revision.Sep 28 2017, 9:17 AM