[PATCH] D41953: [LoopUnroll] Unroll and Jam

[Dave Green via Phabricator via llvm-commits]

dmgreen reopened this revision.
dmgreen added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D41953#1113730, @hfinkel wrote:

> There are a number of obvious enhancements that might be worthwhile, but I think it's best to work on those after this lands.


Any suggestions on what would be at the top of the list? :)

In https://reviews.llvm.org/D41953#1114289, @hsaito wrote:

> David, outer loop vectorization we've been working on and unroll and jam have a lot of similarities and thus there should be code sharing opportunities around legality and cost modeling. Let's collaborate to improve both.


Yeah, sounds like a good idea. Vectorisation is not something I know a huge amount about, it not being useful for the particular cores I'm looking at at the moment. But the work on VPlan sounds interesting and I do wish we had something similar for loop transforms in llvm. I was hoping to get loop versioning working, maybe through LoopAccessAnalysis, but as far as I understand it doesn't handle outer loops yet (and seemed very vectoriser shaped when I looked). From what I remember hearing, this isn't part of the outer loop vectorisation work (instead relying on user directives?). I imagine it's not an easy task to extend this out.

The current legality checks here are based on dependence analysis, and there are a couple of patches needed to make it work correctly. The cost modelling is very adhoc. :) All these things I hope we can improve as we go along.

In https://reviews.llvm.org/D41953#1113514, @echristo wrote:

> From a quick glance it also looks like the testcases could be cleaned up a bit. In particular the naming if nothing else.


Do you mean names of the test files, or names of variables in the tests? I had look but didn't feel I was making things much better (apart from the names of some lcssa nodes). Any other suggestions?



================
Comment at: llvm/trunk/lib/Transforms/Utils/LoopUnrollAndJam.cpp:726
+  const SCEV *TripCountSC =
+      SE.getAddExpr(BECountSC, SE.getConstant(BECountSC->getType(), 1));
+  if (isa<SCEVCouldNotCompute>(TripCountSC))
----------------
hfinkel wrote:
> Can this overflow if the exit count is INT_MAX (or similar)? It's generally a good idea to comment on how that's handled.
Yep, you are correct. This code comes from unroll-runtime (minus the missing overflow check), and dates back to before unroll-and-jam was a separate pass. I was trying to prevent this function from returning true if the runtime unrolling was going to fail. It can now be safely removed and leave that check to the call to UnrollRuntimeLoopRemainder.


Repository:
  rL LLVM

https://reviews.llvm.org/D41953