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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87679