This is an archive of the discontinued LLVM Phabricator instance.

[LoopVectorizer] Support predicating instructions in blocks with several input edges
ClosedPublic

Authored by mkuper on Aug 29 2016, 3:32 PM.

Details

Summary

It's not really clear to me (or to Gil, who suggested the fix) why this was restricted to single-entry blocks to begin with.

This fixes PR30172.

Diff Detail

Event Timeline

mkuper updated this revision to Diff 69625.Aug 29 2016, 3:32 PM
mkuper retitled this revision from to [LoopVectorizer] Support predicating instructions in blocks with several input edges.
mkuper updated this object.
mkuper added reviewers: mssimpso, gilr.
mkuper added a subscriber: llvm-commits.
mssimpso edited edge metadata.Aug 30 2016, 7:32 AM

This looks reasonable to me, but I have a question about the test case (see the inline comment). I too cannot say why the original conditional stores patch limited this to single-predecessor blocks.

test/Transforms/LoopVectorize/if-pred-non-void.ll
197

Is the test checking what you want it to? I was expecting if.then to have more than one predecessor (and trigger the assert) since it's the block with the sdiv's.

mkuper added inline comments.Aug 30 2016, 8:11 AM
test/Transforms/LoopVectorize/if-pred-non-void.ll
197

You're completely right - I was tweaking the test to make the checks more readable, and didn't notice I no longer had the right structure.
Will fix it, thanks!

gilr added inline comments.Aug 30 2016, 9:45 AM
test/Transforms/LoopVectorize/if-pred-non-void.ll
195

It might make the test a bit more future-proof to base the branch on a non loop-invariant condition - make it more resistant to versioning/unswitching.

mkuper updated this revision to Diff 69721.Aug 30 2016, 10:22 AM
mkuper edited edge metadata.

Now, with a correct test.

mssimpso accepted this revision.Aug 30 2016, 11:16 AM
mssimpso edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Aug 30 2016, 11:16 AM
gilr accepted this revision.Aug 30 2016, 1:08 PM
gilr edited edge metadata.

LGTM - Thanks, Michael!

This revision was automatically updated to reflect the committed changes.