Page MenuHomePhabricator

Enable LoopVectorization by default.
ClosedPublic

Authored by asbirlea on Apr 24 2019, 2:01 PM.

Details

Summary

When refactoring vectorization flags, vectorization was disabled by default in the new pass manager.
This patch re-enables is for both managers, and changes the assumptions opt makes, based on the new defaults.

The patch also removes the -disable-loop-vectorization flag and leaves a single way to force *disable* vectorization for -O[23s] (via -vectorize-loops=false). We can add a cl::opt to force enabling under -O[01z] if anyone actually needs this.
Comments in opt.cpp should also help clarify the intended use of the flags to enable/disable vectorization.

Diff Detail

Repository
rL LLVM

Event Timeline

asbirlea created this revision.Apr 24 2019, 2:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2019, 2:01 PM
Herald added a subscriber: jlebar. · View Herald Transcript
chandlerc added inline comments.Apr 24 2019, 2:54 PM
tools/opt/opt.cpp
385–388 ↗(On Diff #196517)

"specific user" doesn't make sense here to me.

Not sure why it makes sense for DisableLoopVectorization to be true by default...

I'm not super worried about the existing behavior of cl::opt flags. Those aren't stable. So I'd suggest picking names and set of such flags that are simple and easy to explain.

Having both -- -disable-loop-vectorization and -vectorize-loops -- is somewhat confusing. Could we have a tri-state cl::opt which, when not specified, uses OptLevel > 1 && SizeLevel < 2 dis-/enable the vectorizer? Similar to -disable-loop-unrolling.

asbirlea updated this revision to Diff 196555.Apr 24 2019, 4:53 PM

Remove DisableLoopVectorization flag.
This leaves a single way to force-enable vectorization for O1/Oz (via -loop-vectorize). Update metadata-enable.ll test which was relying on this.

We may want to re-add a ForceEnable flag that behaves as the expected O1VEC and OzVEC prefixes in the metadata-enable.ll test?

asbirlea updated this revision to Diff 196557.Apr 24 2019, 5:09 PM

Minor cleanups.

chandlerc accepted this revision.Apr 24 2019, 5:21 PM

I like the patch, LGTM.

Slight correction to your email for posterity:

Remove DisableLoopVectorization flag.
This leaves a single way to force-enable vectorization for O1/Oz (via -loop-vectorize).

I think you mean (and your patch does), a single way to force *disable* vectorization for -O[23s] (via -vectorize-loops=false).

And we can add a cl::opt to force enabling under -O[01z] if anyone actually needs this.

This revision is now accepted and ready to land.Apr 24 2019, 5:21 PM
asbirlea edited the summary of this revision. (Show Details)Apr 24 2019, 9:46 PM

Yes, exactly! I updated the summary to include this. Thank you for the review!

This revision was automatically updated to reflect the committed changes.