This is an archive of the discontinued LLVM Phabricator instance.

Modify interleave diagnostics to clearly indicate why interleaving wasn't done.
ClosedPublic

Authored by tyler.nowicki on Jun 24 2015, 3:00 PM.

Details

Reviewers
hfinkel
Summary

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.

Diff Detail

Event Timeline

tyler.nowicki retitled this revision from to Modify interleave diagnostics to clearly indicate why interleaving wasn't done..
tyler.nowicki updated this object.
tyler.nowicki edited the test plan for this revision. (Show Details)
tyler.nowicki added a reviewer: hfinkel.
tyler.nowicki added a subscriber: Unknown Object (MLST).

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.

Gerolf added a subscriber: Gerolf.Jul 7 2015, 12:31 PM
Gerolf added inline comments.
lib/Transforms/Vectorize/LoopVectorize.cpp
1651

It would be easier to read if you handled the checks separately

1678

add assert(IC > 1).

Thanks for the review Gerolf. I've expanded / separate the diagnostic section.

hfinkel added inline comments.Jul 9 2015, 6:52 PM
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

Gerolf added inline comments.Jul 10 2015, 2:14 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
1573

The comment in DEBUG ('already vectorized') is inconsistent with your comment.

1576–1578

Could there be two separated remarks something like a) not vectorized because turned off and b) not vectorized because of low thresholds

tyler.nowicki marked 2 inline comments as done.EditedJul 10 2015, 3:41 PM

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.

With one FIXME LGTM.

lib/Transforms/Vectorize/LoopVectorize.cpp
1573

Ok. Please keep that thought in a FIXME.

1576–1578

Ok, thanks.

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

hfinkel accepted this revision.Aug 9 2015, 9:20 PM
hfinkel edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Aug 9 2015, 9:20 PM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in r244485.