This is an archive of the discontinued LLVM Phabricator instance.

[runtime] Move prolog/epilog block to a post-simplify strategy
ClosedPublic

Authored by reames on Aug 22 2021, 1:12 PM.

Details

Summary

The runtime unroller will try to produce a non-loop if the unroll count is 2 and thus the prolog/epilog loop would only run at most one iteration. The old implementation did this by avoiding loop construction entirely. This patches instead constructs the trivial loop and then explicitly breaks the backedge and simplifies. This does result in some additional code churn when triggered, but a) results in better quality code and b) removes a codepath which didn't work properly for multiple exit epilogs.

One oddity that I want to draw to reviewer attention is that this somehow changes revisit order. The new order looks equivalent to me, but I don't understand how creating and erasing an extra loop here creates this effect.

Diff Detail

Event Timeline

reames created this revision.Aug 22 2021, 1:12 PM
reames requested review of this revision.Aug 22 2021, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2021, 1:12 PM
anna edited reviewers, added: anna; removed: annatom.Aug 23 2021, 10:05 AM
anna added a comment.Aug 25 2021, 7:09 AM

Hi Philip, couple of comments inline.

llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp
949

Could you move this code above the EXPENSIVE CHECKS since there is DT updates here?
We can have full verification of DT in that case.

952

Pls add an assert that RemainderLatch exists (i.e. there is exactly one latch).

There are couple of conditions required by breakLoopBackedge. Is CloneLoopBlocks guaranteed to take care of those conditions (preheader, single latch etc)?

955

As a side-note, not relevant to this patch - we would need to add MSSA support if runtime unroll is used in an LPM with other passes that preserve MSSA.

963

Purely stylistic: I usually prefer the iterator increment in the for loop as ++I and then:

Instruction *Inst = &*I;
llvm/lib/Transforms/Utils/LoopUtils.cpp
743

Stray change? Can be landed separately?

reames added inline comments.Aug 27 2021, 9:24 AM
llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp
949

I'm happy to duplicate the expensive check if desired, but having it after manual DT update, and before well tested utilities which assume an up to date DT on entry makes sense to me.

952

Yes, these preconditions are implied. Will add the requested assert.

963

I would strongly prefer to retain this style. It's idiomatic in the codebase.

llvm/lib/Transforms/Utils/LoopUtils.cpp
743

Can be landed separately, will do.

reames updated this revision to Diff 369146.Aug 27 2021, 11:11 AM

Rebase and address a couple of review comments.

anna accepted this revision.Aug 31 2021, 8:07 AM

LGTM.

llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp
949

ah got it.

This revision is now accepted and ready to land.Aug 31 2021, 8:07 AM
This revision was landed with ongoing or failed builds.Aug 31 2021, 9:31 AM
This revision was automatically updated to reflect the committed changes.