[PATCH] D17526: [LoopUnroll] Respect the convergent attribute.

[Justin Lebar via llvm-commits]

jlebar added inline comments.

================
Comment at: lib/Transforms/Utils/LoopUnroll.cpp:275-278
@@ -263,1 +274,6 @@
+  );
+  // Don't output the runtime loop prolog if Count is a multiple of
+  // TripMultiple.  Such a prolog is never needed, and is unsafe if the loop
+  // contains a convergent instruction.
+  if (RuntimeTripCount && TripMultiple % Count != 0 &&
       !UnrollRuntimeLoopProlog(L, Count, AllowExpensiveTripCount, LI, SE, DT,
----------------
mzolotukhin wrote:
> jlebar wrote:
> > mzolotukhin wrote:
> > > I think this should go in a separate commit.
> > Since you approved this patch, I'm going to submit as one commit, I hope that's OK.  Other passes already remove said prelude in the non-convergent case -- the only reason I think you'd want this change is if you're also taking the rest of this patch.
> > 
> > Maybe if this were a huge change and there were some readability benefit to be had from splitting it up, but that also doesn't seem to be the case here.  Keeping this all together also makes our life a bit easier if we have to revert (which is possible if this prolog removal is somehow wrong).
> The reason I proposed it is that this is a change that touches general code, while all the rest is here to deal with 'convergent' attribute. If for some (theoretical) reason we later decide to revert the 'convergent' part, we still might want to keep the generic part. But that's unlikely to actually happen so yeah, you an go ahead with a single patch.
Ah, interesting, you saw this in the reverse way!

Thanks, just submitted.  And thank you very much for the review -- it's much appreciated.


Repository:
  rL LLVM

http://reviews.llvm.org/D17526