[PATCH] D43293: [SimplifyCFG] Don't remove preheaders when we need canonical loops.

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Hello all

Yes, from https://reviews.llvm.org/rL324572 I'm seeing multiple small regressions and two large-ish ones (no improvements unfortunately - I usually try to look for reasons why changes like that are a net gain, even if they cause some regressions, but in this case my numbers are not being very kind in that regard). I believe removing preheaders was not intended, and this "fixes" all of the regressions except for one of the big ones. That looks like it's to do with loop rotate choosing a differently loop header, and the knock on affects of that. I'm still trying to pull apart why exactly that is making things worse there.

For this patch - which was suggested by Serguie as a potential fix (I should have mentioned that somewhere - sorry). I agree that if we need loop simple form then that pass will re-create the loop preheader for us. So the change in https://reviews.llvm.org/rL324572 to remove preheaders should be benign, except for block layout, anything that might benefit from less critical edges and the fact that we are repeatedly removing and recreating loop preheaders through the pass pipeline. The NeedCanonicalLoop option seems to be requiring that we don't break loop simple form, which this patch aims to fix. So not a direct fix (I'm not sure that would even be possible for all these regressions) but hopefully a sensible change none-the-less (?)



================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5747
+  // introduce new backedge, so we can eliminate BB.
   bool NeedCanonicalLoop =
+      Options.NeedCanonicalLoop && LoopHeaders &&
----------------
skatkov wrote:
> It becomes complicated. Could you please re-write it in a more readable way?
> 
> I guess lambda with explicit ifs and possible comments may do things cleaner in terms of reading this.
I agree this one was giving me trouble. Thanks for the lambda suggestion, that's a good idea.


https://reviews.llvm.org/D43293