This is an archive of the discontinued LLVM Phabricator instance.

Addition in Auto Vectorization Cost-Model Remark
Needs ReviewPublic

Authored by cs15btech11042 on Mar 2 2018, 11:40 PM.

Details

Summary

This is a little addition in the current version of vectorization cost-model remark . Earlier cost-model remark used to say that "the cost-model indicates that vectorization is not beneficial" . Now, before this remark , it also prints the cost of scalar loop and the cost of best vector loop possible so as to notify the programmer about the amount of benefit he/she may get on vectorizing or not vectorizing the loop . In case no vectorization is possible for any of the width , it says that " none of the width generate any vector instructions ".

Diff Detail

Event Timeline

cs15btech11042 created this revision.Mar 2 2018, 11:40 PM
cs15btech11042 updated this revision to Diff 138395.EditedMar 14 2018, 9:55 AM

Corrections to the code as per LLVM coding standards .

cs15btech11042 retitled this revision from Modified Cost-Model Remark to Addition in Auto Vectorization Cost-Model Remark.Mar 16 2018, 1:22 AM
cs15btech11042 edited the summary of this revision. (Show Details)
cs15btech11042 added a reviewer: vivekvpandya.
vivekvpandya added subscribers: anemet, nadav.

Final looks good or rejection should be given by community member which have more experience with LoopVectorizer. Adding @nadav , @anemet.

Few high level comments:

  1. Please provide bug (if filed for this) or example for which this improvement helps.
  2. None of the existing LIT test case is affected by this change then it is better to add new LIT test which captures this improvement.
  3. I hope in addition to LIT based testing you have also done unit testing. You need to enable unit testing with cmake while building LLVM source tree with cmake flag LLVM_BUILD_TESTS=On and then with check-all target in LLVM build system, you can use make check-all or ninja check-all for running those tests.
vivekvpandya added inline comments.Mar 16 2018, 3:55 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
6178

no space required before ;
Same comments applies to few other lines below like 6179, 6233, 6240. Please fix all such minor things.

6228

It seems like only remark message is changed based on this condition, then why not create just message string based on this variable and use it in single ORE->emit().

6232

Make sure lines width should no go beyond 80 characters. Configure your editor or IDE to help you for that.

6235

instead of

 }
else {

it should be

} else {

Fixing of some minor coding style mistakes.

lib/Transforms/Vectorize/LoopVectorize.cpp
6178

Sure,will do it . Thanks a lot.

6228

Actually, we can create string and concatenate them to produce a single string . But , I avoided that because I didn't want to create an extra overhead by involving inefficient std::string concatenation operations. Please let me know if I am wrong.

6232

Sure , will do it . Thanks a lot.

6235

Sure , will do it . Thanks a lot.

cs15btech11042 marked 2 inline comments as done.Mar 17 2018, 6:38 AM

In first pass of the review I did not pay attention to logic in this change, but I think this change is not required because as per my understanding from the code the best cost of vectorization is already calculated by the code, when vectorization is not beneficial it just not generating optimization remark, but if when vectorization is forced by user it does print this information in debug output statements. I am referring DEBUG(if (ForceVectorization && Width > 1 && Cost >= ScalarCost).

In this situation we have to wait for any one of the reviewer to take a look.

lib/Transforms/Vectorize/LoopVectorize.cpp
6206

These if block already tracks minimum cost and width associated with that.
I don't understand the purpose of your code change.

test/Transforms/LoopVectorize/X86/print_cost.ll
1

In general comments :
Test for this kind of feature should not be put inside target specific directories.
Also if you see any other tests in LoopVectorize directory like https://github.com/llvm-mirror/llvm/blob/master/test/Transforms/LoopVectorize/no_array_bounds.ll
it has original C++ program written in comments.
Also you should try to reduce IR as much as possible so that only necessary things for testing is written in side the test. For example you can drop triple. You may be able to drop some X86 specific target-features attribute etc.

In first pass of the review I did not pay attention to logic in this change, but I think this change is not required because as per my understanding from the code the best cost of vectorization is already calculated by the code, when vectorization is not beneficial it just not generating optimization remark, but if when vectorization is forced by user it does print this information in debug output statements. I am referring DEBUG(if (ForceVectorization && Width > 1 && Cost >= ScalarCost).

In this situation we have to wait for any one of the reviewer to take a look.

Firstly, the information you said is printed only in debug statements, many of the programmers who build llvm in release build mode won't be able to print it.
Secondly, the information printed in the debug statement here is not the same as I am printing.I am printing the best vectorization cost along with best vector width, as well as the scalarization cost .
Please note that when I use the term vectorization here I mean that width of that vector is greater than 1. Case of width equal to 1 is explicitly stated as scalarization.

lib/Transforms/Vectorize/LoopVectorize.cpp
6206

No, actually this only captures the cost which is best for any width between 1 to MaxVF.

What I am proposing is that let's print the cost of scalarization as well as the cost of best vectorization (from width 2 to MaxVF). This will add verbosity to the remark in a way that now user will know why the loop is not vectorized and can easily let him/her know that how much cost benefit he/she may get on force vectorizing a loop by the best vector width possible.