[PATCH] D32261: [LoopUnroll] Don't try to unroll non-rotated loops

[Michael Zolotukhin via Phabricator via llvm-commits]

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

Thanks for addressing my previous remarks! The change looks good to me (modulo Eli's remark regarding the check).

> I consider not crashing (or ending up with a corrupt dominator) more important in this case, and in this sense, this patch is an improvement compared to what we have today. If we want to improve the algorithm later to handle more sophisticated latches/multiple latches, we might as well do later (and quite frankly, I wouldn't really go that route until we hit a case where this matters).

Your reasoning makes perfect sense to me.

Michael



================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:332
+      });
+  if (NotRotated) {
+    DEBUG(dbgs() << "  Can't unroll; loop not rotated\n");
----------------
davide wrote:
> efriedma wrote:
> > It might be simpler to explicitly check the two successors; one should be the header, and the other outside the loop.
> > 
> > You also can't delete the check that BI is non-null and conditional; we specifically assume that later on.
> Thanks for the comments, I'll change that as I also believe it's simpler. 
> No tests are failing if I remove that check, which is slightly concerning.
> I'll try to craft a test where this fails.
I agree that an explicit check would be simpler and easier to understand here.


https://reviews.llvm.org/D32261