This is an archive of the discontinued LLVM Phabricator instance.

Restructuring LoopRotation.cpp to create Loop Rotation Pass with Loop Rotation Utility Interface
ClosedPublic

Authored by jinlin on Mar 16 2018, 5:40 PM.

Details

Summary

The existing LoopRotation.cpp is implemented as one of loop passes instead of being a utility. The user cannot easily perform the loop rotation selectively (or on demand) under different optimization level. For example, the loop rotation is needed as part of the logic to convert a loop into a loop with bottom test for a transformation. If the loop rotation is simply added as a loop pass before the transformation, the pass is skipped if it is compiled at –O0 or if it is explicitly disabled by the user, causing the compiler to generate incorrect code. Furthermore, as a loop pass it will rotate all loops instead of just the relevant loops.

We provide a utility interface for the loop rotation so that the loop rotation can be called on demand. The changeset is as follows:

  1. Create a new file lib/Transforms/Utils/LoopRotationUtils.cpp and move the main implementation of class LoopRotate into this file.
  2. Create a new file llvm/include/Transform/Utils/LoopRotationUtils.h with the interface LoopRotation(…).
  3. Original LoopRotation.cpp is changed to use the utility function LoopRotation in LoopRotationUtils.cpp. This is done in the same way community did for mem-to-reg implementation.

Diff Detail

Event Timeline

jinlin created this revision.Mar 16 2018, 5:40 PM

Please upload all patches with full context (-U999999).

jinlin updated this revision to Diff 139316.Mar 21 2018, 10:02 AM
jinlin updated this revision to Diff 139317.Mar 21 2018, 10:06 AM
lebedev.ri added inline comments.Mar 21 2018, 10:17 AM
include/llvm/Transforms/Utils/LoopRotationUtils.h
2

Please fix the first line

lib/Transforms/Utils/LoopRotationUtils.cpp
21

This should probably be in the differential's description, not as comment in the source code.

jinlin added a reviewer: lebedev.ri.
jinlin marked an inline comment as done.Mar 21 2018, 3:35 PM

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

include/llvm/Transforms/Utils/LoopRotationUtils.h
2

Done.

jinlin updated this revision to Diff 139384.Mar 21 2018, 3:50 PM
jinlin updated this revision to Diff 139385.Mar 21 2018, 3:52 PM
jinlin updated this revision to Diff 139386.Mar 21 2018, 3:54 PM

Seems like a sensible move.

lib/Transforms/Scalar/LoopRotation.cpp
42

Can some of these headers now be removed?

jinlin marked an inline comment as done.
jinlin updated this revision to Diff 139529.Mar 22 2018, 3:46 PM
dmgreen accepted this revision.Mar 23 2018, 9:47 AM

Thank. LGTM

lib/Transforms/Utils/LoopRotationUtils.cpp
40

I believe this header has moved to Analysis/Utils/Local.h

This revision is now accepted and ready to land.Mar 23 2018, 9:47 AM
jinlin added inline comments.Mar 23 2018, 12:57 PM
lib/Transforms/Utils/LoopRotationUtils.cpp
40

The header file path is correct.

/export/iusers/XXX/v15/llvm/include/llvm=>find . -name "Local.h"
./Transforms/Utils/Local.h

dmgreen added inline comments.Mar 24 2018, 2:54 AM
lib/Transforms/Utils/LoopRotationUtils.cpp
40

https://reviews.llvm.org/rL328165 is the commit I was thinking about. It only went in a couple of days ago.

jinlin added inline comments.Mar 26 2018, 10:04 AM
lib/Transforms/Utils/LoopRotationUtils.cpp
40

Hi Dave.

Thanks so much for your update. I will update the header file on it.

Thanks,

Jin

jinlin retitled this revision from Restructuring LoopRoration.cpp to create Loop Rotation Pass with Loop Rotation Utility Interface to Restructuring LoopRotation.cpp to create Loop Rotation Pass with Loop Rotation Utility Interface.Mar 26 2018, 12:47 PM
jinlin updated this revision to Diff 139838.Mar 26 2018, 12:49 PM
jinlin added inline comments.Mar 26 2018, 5:21 PM
lib/Transforms/Utils/LoopRotationUtils.cpp
40

Hi Dave,

The changes have passed with the tests. However, I don't have privilege to check in the changes. Would you please do me a favor to check in the changes?

Thanks,

Jin

Sounds like a great excuse to get commit access ;)

But yes, sure, I can commit it tomorrow morning if you like.

Thank you Dave. I will apply for the commit access soon.

Jin

Sounds like a great excuse to get commit access ;)

But yes, sure, I can commit it tomorrow morning if you like.

Hi Dave,

Would you please help me to commit the changes? I have not got the approval email from Chris yet.

Thanks,

Jin

This revision was automatically updated to reflect the committed changes.

Sorry for the delay. I hope it looks OK. Let me know if not.

efriedma added inline comments.
llvm/trunk/include/llvm/Transforms/Utils/LoopRotationUtils.h
31 ↗(On Diff #140193)

This seems like a bad API for a utility. It doesn't really rotate a loop; it implements the entire loop rotation pass. It performs multiple different transforms, some of which don't actually rotate the loop. And there are a bunch of profitability heuristics which aren't under the control of this API. Overall, the caller doesn't really know what will happen to the loop. I don't understand how anyone could use this API to do anything useful besides implement the -loop-rotate pass.

jinlin added inline comments.Apr 6 2018, 7:51 AM
llvm/trunk/include/llvm/Transforms/Utils/LoopRotationUtils.h
31 ↗(On Diff #140193)

I will prepare another change set to address the issues you pointed out.