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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5605
     MaxInterleaveCount =
         std::min(*BestKnownTC / VF.getKnownMinValue(), MaxInterleaveCount);
   }
----------------
ebrevnov wrote:
> 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;
> }
> 
> 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:

I don't think there's anything currently preventing targets to return 0 for the max interleave factor, but I think it is reasonable to require them to return a MaxInterleaveFactor >= 1 and assert otherwise.

But I think it should be sufficient to make sure that max IC >= 1 here with a comment, as Ayal suggested


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5617
   // that the target and trip count allows.
   if (IC > MaxInterleaveCount)
     IC = MaxInterleaveCount;
----------------
ebrevnov wrote:
> 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?
Given that @Ayal also seems to prefer sanitizing the value earlier I think it would be preferable to do so earlier, but don't have a strong opinion on that. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87679