[PATCH] D87679: [LV] Unroll factor is expected to be > 0

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5617
   // that the target and trip count allows.
   if (IC > MaxInterleaveCount)
     IC = MaxInterleaveCount;
----------------
ebrevnov wrote:
> Ayal wrote:
> > fhahn wrote:
> > > ebrevnov wrote:
> > > > fhahn wrote:
> > > > > IIUC the problem here is in cases when `MaxInterleaveCount == 0`, right? 
> > > > > 
> > > > > The code below expects MaxInterleaveCount > 0 and I think it would be slightly preferable to make sure `MaxInterleaveCount` is at least 1. With that, the code setting `IC` should do the right thing, and other (potential future) users of `MaxInterleaveCount` can expect a sane value.
> > > > > IIUC the problem here is in cases when `MaxInterleaveCount == 0`, right? 
> > > > That's right.
> > > > 
> > > > > 
> > > > > The code below expects MaxInterleaveCount > 0 and I think it would be slightly preferable to make sure `MaxInterleaveCount` is at least 1.
> > > > I don't see any uses of MaxInterleaveCount  bellow this point.
> > > > 
> > > > 
> > > >> The code below expects MaxInterleaveCount > 0 and I think it would be slightly  preferable to make sure MaxInterleaveCount is at least 1.
> > > > I don't see any uses of MaxInterleaveCount bellow this point.
> > > 
> > > Right, personally I think it would still be slightly preferable to ensure the variables have sane values as early as possible, especially when a `MaxInterleaveCount ` of 0 does not really make sense in this context and makes the existing code for IC handling work as expected. But it probably it doesn't make much of a difference in practice.
> > > 
> > > Also, using `std::max` would seem slightly more explicit to me, together with a comment that IC must be at least 1.
> > > personally I think it would still be slightly preferable to ensure the variables have sane values as early as possible, especially when a MaxInterleaveCount  of 0 does not really make sense in this context and makes the existing code for IC handling work as expected ...
> > 
> > Agreed. Adding an assert that MaxInterleaveCount is strictly positive is also recommended.
> > >> The code below expects MaxInterleaveCount > 0 and I think it would be slightly  preferable to make sure MaxInterleaveCount is at least 1.
> > > I don't see any uses of MaxInterleaveCount bellow this point.
> > 
> > Right, personally I think it would still be slightly preferable to ensure the variables have sane values as early as possible, especially when a `MaxInterleaveCount ` of 0 does not really make sense in this context and makes the existing code for IC handling work as expected. But it probably it doesn't make much of a difference in practice.
> What I don't like in this approach is over complication. That doesn't help code readability. Instead of one line IC = std::max(1, IC) which 
> clearly dominates all future uses of IC we have to sanitize MaxIC first, then do conditional sanitizing of IC and finally assert IC value. If you worry about using note sanitized MaxIC value I would suggest putting it to a separate scope. Like this:
> 
> {
>   unsigned MaxIC=V1;
>   if (c1)
>     MaxIC=V2;
>   ...
>   if (IC > MaxIC)
>     IC = MaxIC
> }
> 
> IC = std::max(1, IC);
> 
> What do you think?
> 
> > 
> > Also, using `std::max` would seem slightly more explicit to me, together with a comment that IC must be at least 1.
> Agreed.
> 
> 
@fhahn what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87679