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 ".
Details
Diff Detail
Event Timeline
Final looks good or rejection should be given by community member which have more experience with LoopVectorizer. Adding @nadav , @anemet.
Few high level comments:
- Please provide bug (if filed for this) or example for which this improvement helps.
- 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.
- 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.
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
6178 | no space required before ; | |
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. |
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. | |
test/Transforms/LoopVectorize/X86/print_cost.ll | ||
1 | In general comments : |
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. |
no space required before ;
Same comments applies to few other lines below like 6179, 6233, 6240. Please fix all such minor things.