[PATCH] D22630: Loop rotation

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Eli,

Since I worked on a similar patch, I completely understand what Aditya and Sebastian are trying to say, so let me share my perspective on this too.

The existing version of loop-rotate is very limited, essentially it only works on loops with two basic blocks. And it's not heuristics, it's just a limitation of implementation. This patch makes implementation more generic, with it we can handle more complicated loops. Originally Aditya left some of the checks from the original code, which might limited the new implementation to the same 2-blocks loops, but we rightfully asked if these checks are relevant now (I'd say we don't need to add them just to constrain the algorithm). And it's hard to split this patch into several: probably, some cosmetic changes might be split, but there will be one big part anyway - the part that changes the algorithm.

Adding it under a flag sounds like a safe decision, but frankly I don't see much benefits in it: at some point we'll still need to switch the default value of the flag (and then remove the old implementation). I don't see why that decision would be easier than a decision to switch implementations now.

As for the heuristics: we do need to be careful about what loops we rotate and what loops we don't rotate. I think a good starting point for this would be: rotate all loops with non-exiting latch so that the earliest exit becomes a new exiting latch (under condition that the size of copied code is less than the threshold). I haven't examined what exactly we're doing in this patch in this aspect now, but I'm sure it's relatively easy to do this in this framework.

Thanks,
Michael


https://reviews.llvm.org/D22630