Currently the message that is produced implies that the user disabled interleaving. This isn't always true however. Sometimes interleaving it is not beneficial, as determined by the vectorizers cost model. With this patch the interleaving profitability and disabled state of the optimization are separately indicated by the message.
Details
Diff Detail
Event Timeline
In this update, and the previous update, renaming parts of the work are removed/moved to the renaming patch.
Sorry about the spam. I am getting used to phabricator.
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
1679 | These variable should start with a capital letter (per the coding conventions). | |
1682 | Same here (and below)... capital letters at the start of variable names. | |
1691 | Saying "is beneficial" seems a bit strong, especially considering that the user explicitly turned it off. We could say, "The cost model indicates that interleaving is beneficial, but...". |
Hi Hal,
Thanks for the review, and sorry for the variable name mistakes.
Since the diagnostic messages are a little longer now I generate two separate messages for vectorization and interleaving rather than concatenating them together.
Here is the updated patch.
Tyler
Hi Gerolf,
Thanks for the comments. I've replied inline.
Tyler
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
1573 | The 'already vectorized' comment is there because the vector width and interleave count are set to 1 after the loop is vectorized. If the loop is, for some reason, run through the vectorizer again then we won't try to vectorize it again. I think a separate fix is needed for this. I think we may need to add metadata like loop.vectorize.complete = true to do this instead. | |
1576–1578 | The metadata doesn't currently allow us to differentiate between those cases. The pragmas vectorization(disable) and vectorization_width(1) do the same thing. They set loop.vectorize.width = 1. This is because loop.vectorize.enable is used disable both vectorization and interleaving. I was working on a patch to change this. However, without diagnostics like this there was no way to test it. So I stopped working on that and moved to this. I'll differentiate between these cases in a subsequent patch. |
Hi Hal,
I updated the comments as Gerolf requested. Please let me know if you have any other comments.
Thanks,
Tyler
I think all comments including Hal’s have been addressed. Adding Michael and/or Arnold to double check.
-Gerolf
The comment in DEBUG ('already vectorized') is inconsistent with your comment.