Page MenuHomePhabricator

[clang] Pragma vectorize_width() implies vectorize(enable)
ClosedPublic

Authored by SjoerdMeijer on Aug 15 2019, 6:11 AM.

Details

Summary

Specifying the vectorization width was supposed to implicitly enable
vectorization, except that it wasn't really doing this. It was only
setting the vectorize.width metadata, but not vectorize.enable.

This should also fix PR27643.

Diff Detail

Repository
rL LLVM

Event Timeline

SjoerdMeijer created this revision.Aug 15 2019, 6:11 AM
fhahn added a comment.Aug 15 2019, 6:16 AM

As also pointed out in the discussion on the cfe dev list, this is probably a bit
of a silly combination:

    
vectorize(enable) vectorize_width(1)

but it could still mean that the vectorizer interleaves. So, with this
simplification, disabled means disabled, and a width of 1 a width of 1.

It would be good to have a test case for that.

SjoerdMeijer edited the summary of this revision. (Show Details)

Thanks for looking again!
Good catch, feedback addressed.

(forgot to add this message when I uploaded the new diff)

I think it would be slightly better to split off the change to disable vectorization via llvm.loop.vectorize.enable=false instead of width=1. This changes the behaviour from "disable vectorization, but allow interleaving in the vectoriser" to "disable the vectoriser". IMO this change makes sense, but it is probably better/safer to do it separately.

Thanks, and sorry for the delay. Back in the office now, and I am addressing this:

I think it would be slightly better to split off the change to disable vectorization via llvm.loop.vectorize.enable=false instead of width=1.

Yep, I agree

I think it would be slightly better to split off the change to disable vectorization via llvm.loop.vectorize.enable=false instead of width=1.

This is now D66796.

I will now start stripping it out from this patch.

SjoerdMeijer edited the summary of this revision. (Show Details)

Stripped out the functional change related to vectorize(disable).

fhahn accepted this revision.Tue, Aug 27, 9:32 AM

LGTM

This revision is now accepted and ready to land.Tue, Aug 27, 9:32 AM
SjoerdMeijer updated this revision to Diff 220446.EditedTue, Sep 17, 1:36 AM

Just uploading new diff for completeness; I only had to change a test-case, and thus thought that committing this is okay.

Many thanks again for reviewing and helping with the discussions!

SjoerdMeijer edited the summary of this revision. (Show Details)Tue, Sep 17, 1:41 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 17, 1:42 AM