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

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5605
     MaxInterleaveCount =
         std::min(*BestKnownTC / VF.getKnownMinValue(), MaxInterleaveCount);
   }
----------------
fhahn wrote:
> 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
I have changed the Hexagon code to return 1 (https://reviews.llvm.org/rG99cafe009477).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87679