This is an archive of the discontinued LLVM Phabricator instance.

Refine the loop rotation's API
ClosedPublic

Authored by jinlin on Apr 12 2018, 11:19 AM.

Details

Summary

The following changes addresses the following two issues.

  1. The existing loop rotation pass contains both loop latch simplification and loop rotation. So one flag RotationOnly is added to be passed to the loop rotation pass.
  2. The threshold value is initialized with MAX_UINT since the loop rotation utility should not have threshold limit.

Diff Detail

Event Timeline

jinlin created this revision.Apr 12 2018, 11:19 AM
efriedma added inline comments.Apr 12 2018, 12:36 PM
include/llvm/Transforms/Utils/LoopRotationUtils.h
29

It would be better to have a separate API for latch simplification, rather than a boolean parameter to LoopRotation.

lib/Transforms/Utils/LoopRotationUtils.cpp
216

This is a heuristic.

227

This is a heuristic.

539

This is a heuristic.

jinlin added inline comments.Apr 12 2018, 4:45 PM
include/llvm/Transforms/Utils/LoopRotationUtils.h
29

If someone is interested in having a new API for latch simplification, he can feel free to add it. The purpose of my change is to provide an API for the loop rotation.

lib/Transforms/Utils/LoopRotationUtils.cpp
216

This is not heuristic. According to the comments, the loop header is not loop exit means that the loop has been rotated or is not suitable for loop rotation.

227

I believe the condition L->isLoopExiting(OrigLatch) is always false at this place.

However, I can add one more flag to indicate that the profitability check should be skipped here.

539

This is not heuristic. The reason the loop rotation gives up at this place is because the implementation of loop rotation has some limitation and it cannot convert this kind of loop into do-while loop.

jinlin updated this revision to Diff 142300.Apr 12 2018, 4:46 PM
efriedma added inline comments.Apr 13 2018, 12:48 PM
include/llvm/Transforms/Utils/LoopRotationUtils.h
29

Even if you don't actually need the latch-simplification API, it would make the rotation API easier to understand. But you can leave it like this for now.

The new arguments need to be documented.

lib/Transforms/Utils/LoopRotationUtils.cpp
216

You're right, sorry. (I guess you technically could clone the header in other cases, but it wouldn't really be "rotation" in the traditional sense.)

539

Checking anything other than isSafeToSpeculativelyExecute() is a heuristic; the actual transform doesn't depend on anything else. That said, I guess it isn't useful to control this heuristic if you're not going to separate out the latch simplification API.

jinlin updated this revision to Diff 142493.Apr 13 2018, 4:44 PM
jinlin added inline comments.
include/llvm/Transforms/Utils/LoopRotationUtils.h
29

Many thanks for your review. I have updated the comments.

lib/Transforms/Utils/LoopRotationUtils.cpp
539

I see. Many thanks for your explanation.

jinlin retitled this revision from change the api in the file LoopRotationUtils.h to change the API in the file LoopRotationUtils.h.Apr 13 2018, 4:49 PM
jinlin retitled this revision from change the API in the file LoopRotationUtils.h to Refine the loop rotation's API.

There is no progress with this review. I wonder whether I need to provide some additional information to move things forward.

This revision is now accepted and ready to land.Apr 17 2018, 11:44 AM

Thanks Eli for your review.

This revision was automatically updated to reflect the committed changes.