This is an archive of the discontinued LLVM Phabricator instance.

[LV] Optimize trip count SCEV.
ClosedPublic

Authored by craig.topper on Mar 31 2023, 1:27 PM.

Details

Summary

To calculate the trip count we need to add 1 to the backedge
taken count. If we need to widen the backedge count, it's better
to do the add before the widening if we can guarantee it won't
overflow.

The code is here is based on similar code I found in
LoopIdiomRecognize.

This is the vectorizer version of this InstCombine patch D142783.

Looking at the IR diffs, this does look like it gets more cases
than the InstCombine patch.

I'm not an expert on SCEV so perhaps this should be handled somewhere
else so that LoopIdiomRecognize can share.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 31 2023, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 1:27 PM
craig.topper requested review of this revision.Mar 31 2023, 1:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 1:27 PM
craig.topper added inline comments.Mar 31 2023, 1:29 PM
llvm/lib/Transforms/Vectorize/VPlan.h
80

There's another call to createTripCountSCEV in VPlanTransforms and it wasn't obvious if it was possible to get the Loop there.

reames requested changes to this revision.Apr 12 2023, 10:35 AM
reames added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1005

I don't believe the flags usage here is correct. There's a single SCEV node for an add with given operands. The flags must be correct for all users of that add expression, and for a loop invariant trip count there's nothing preventing another use outside the loop which does overflow.

This revision now requires changes to proceed.Apr 12 2023, 10:35 AM
craig.topper added inline comments.Apr 12 2023, 10:42 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1005

Is the usage in LoopIdiomRecognize wrong too?

reames added inline comments.Apr 12 2023, 10:51 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1005

Looks that way. There's might be some non-obvious invariant which lets it be correct, but it's probably just wrong.

Remove NUW flag

reames accepted this revision.Apr 12 2023, 11:57 AM

LGTM

This revision is now accepted and ready to land.Apr 12 2023, 11:57 AM

This seems like basically the same change as D147117, but in a different place. It would be nice if every place computing a trip count from a BE count didn't reimplement this.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1002

Can use SE.getMinusOne() here.

Use getMinusOne

This revision was landed with ongoing or failed builds.Apr 12 2023, 4:19 PM
This revision was automatically updated to reflect the committed changes.