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

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

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



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87679