[PATCH] D151052: [LoopUnroll] Peel iterations based on select conditions

[Nikita Popov via Phabricator via llvm-commits]

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

LGTM



================
Comment at: llvm/lib/Transforms/Utils/LoopPeel.cpp:352
+    MaxPeelCount =
+        std::min((unsigned)SC->getAPInt().getZExtValue() - 1, MaxPeelCount);
+
----------------
caojoshua wrote:
> nikic wrote:
> > The getZExtValue() here can assert if we the BECount is larger than 64 bits. You can use getLimitedValue() instead.
> > 
> > > When peeling only branch instructions, we avoid this by ignoring the branch condition of the latch block. But with this patch, we don't ignore and/or's that are used by the latch branch.
> > 
> > FWIW, I think that's a problem in your and/or handling: You shouldn't pick up random and/ors, but only those rooted at a br or select.
> > 
> > But anyway, I don't mind adding the check.
> Thanks, using `getLimitedValue()` in newest changes. I added a test for peeling large loops BECounts (>2^64) to precommit tests locally.
> 
> > FWIW, I think that's a problem in your and/or handling: You shouldn't pick up random and/ors, but only those rooted at a br or select.
> 
> Benefit of checking random and/ors is that allows peeling nested and/ors easily. We can also peel for and/ors not used in branches/selects, for example, we might pass a boolean to a function (probably rare in practice though).
> Benefit of checking random and/ors is that allows peeling nested and/ors easily. We can also peel for and/ors not used in branches/selects, for example, we might pass a boolean to a function (probably rare in practice though).

If we wanted to peel such cases, we would just look at all icmps, regardless of where they occur. But we specifically don't want to peel cases like that, because it's not useful. (It does not enable any meaningful follow-up simplification.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151052/new/

https://reviews.llvm.org/D151052