[PATCH] D13402: Refactor loop unrolling pass and add optimization remarks

Michael Zolotukhin via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 16 12:06:28 PDT 2015


mzolotukhin added a subscriber: mzolotukhin.
mzolotukhin added a comment.

Hi Mark,

In general it looks good, but I'd prefer to split it to smaller patches. That is, separate refactoring (which brings no functionality changes) from adding new stuff (that is, printing remarks). The refactoring, in its turn, should be done in as small steps as possible - e.g. 1) factor out `UnrollParameters` class, 2) factor out `UnrollLimits`, etc.

One more question: loop-unrolling is currently executed twice in our O3 pipeline, and in theory it's possible that we'll unroll the same loop twice (e.g. partially the first time and completely the second time). In this case we'll emit two remarks for the same loop, which might be confusing for the user. Any thought on how we can avoid doing this?

Also, please find some comments inline.

Thanks,
Michael


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:83
@@ +82,3 @@
+class LoopDescription;
+class UnrollLimits;
+
----------------
`UnrollLimits` is a struct, not a class, right?

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:248
@@ +247,3 @@
+// decision.
+class LoopDescription {
+public:
----------------
I'm a bit worried about this name, as it's very general and doesn't mention unrolling. For instance, currently we have `LoopInfo` which sounds similar, but holds absolutely different information. I'm not saying we want to merge them in any way, but just mentioned it because it might be confusing for the future readers of the code, since `LoopDecscription` has no hint regarding how specialized it is.


http://reviews.llvm.org/D13402





More information about the llvm-commits mailing list