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

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added a comment.

In D87679#2296877 <https://reviews.llvm.org/D87679#2296877>, @fhahn wrote:

> LGTM, thanks. Suggestion inline, but quite subjective, so feel free to ignore.





================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5605
     MaxInterleaveCount =
         std::min(*BestKnownTC / VF.getKnownMinValue(), MaxInterleaveCount);
   }
----------------
Ayal wrote:
> this is the culprit: `*BestKnownTC < VF.getKnownMinValue()` leads to zeroing MaxInterleaveCount, right?
> 
> Best check here if this is the case, and if so reset MaxInterleaveCount to 1 instead of 0. Also worth updating the above comment.
In this particular case yes, *BestKnownTC < VF.getKnownMinValue() is the problem. But MaxInterleaveCount  can potentially be set to 0 in early assignments as well. Is there any guarantee that user won't set ForceTargetMaxScalarInterleaveFactor/ForceTargetMaxVectorInterleaveFactor to 0? Also I don't see anything preventing TTI::getMaxInterleaveFactor to return 0. In fact there is one target returning 0:

unsigned HexagonTTIImpl::getMaxInterleaveFactor(unsigned VF) {
  return useHVX() ? 2 : 0;
}



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5617
   // that the target and trip count allows.
   if (IC > MaxInterleaveCount)
     IC = MaxInterleaveCount;
----------------
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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87679