[LV] Unroll factor is expected to be > 0

[ebrevnov]

Summary

LV fails with assertion checking that UF > 0. We already set UF to 1 if it is 0 except the case when IC > MaxInterleaveCount. The fix is to set UF to 1 for that case as well.

[fhahn]

Thanks for the patch! Some comments inline.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5617

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.

llvm/test/Transforms/LoopVectorize/zero_unroll.ll

1 ↗

(On Diff #291847)

I think the test needs moving to llvm/test/Transforms/LoopVectorize/SystemZ/, because it relies on SytemZ's cost model and might fail if LLVM is build without the SystemZ backend.

Could you also add a check line to make sure a vector body is generated?

5 ↗

(On Diff #291847)

nit: dso_local local_unnamed_addr unneeded?

25 ↗

(On Diff #291847)

nit: are those attributes needed?

[dmgreen]

llvm/test/Transforms/LoopVectorize/zero_unroll.ll

12 ↗

(On Diff #291847)

This looks like an infinite outer loop? It maybe doesn't matter a large amount, but if it shows the same problem with a ret instead of looping back, that would make a conceptually simpler test.

[ebrevnov]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5617

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.

[ebrevnov]

Addressed comments

[ebrevnov]

Minor change to test case

[ebrevnov]

@fhahn ping

[fhahn]

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

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5617

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.

[Ayal]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5605

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.

5617

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.

[ebrevnov]

In D87679#2296877, @fhahn wrote:

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

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5605

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;

}

5617

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.

[ebrevnov]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5617

@fhahn what do you think?

[fhahn]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5605

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

5617

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.

[ebrevnov]

Sanitize MaxIC upfront.

[ebrevnov]

Does this look fine?

[fhahn]

LGTM, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5607

nit: seems like something like 'count' is missing from the sentence below. Or say something like MaxInterleave count must be at least 1.

[kparzysz]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5605

I have changed the Hexagon code to return 1 (https://reviews.llvm.org/rG99cafe009477).

[Ayal]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5602

My suggestion would be to (1) update the above comment: the interleave count is limited to be less than the trip count divided by VF, provided it is at-least 1.

(2) place the MaxInterleaveCount = std::max(1u, MaxInterleaveCount); fix immediately after setting

MaxInterleaveCount =
    std::min(*BestKnownTC / VF.getKnownMinValue(), MaxInterleaveCount);

and (3) introduce an assert(MaxInterleaveCount > 0 && "Max Interleave Count must be at-least 1"); later -- setting it to 0 by targets should be treated as an error (thanks @kparzysz for the fix!)

[ebrevnov]

Fixed according to Ayal's request.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5617

Done

[Ayal]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5620

nit: suffice to only introduce the

assert(MaxInterleaveCount > 0 &&
       "Maximum interleave count must be greater than 0");

in the above change hunk, right?

Thanks!

[ebrevnov]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

5620

That was done intentionally. By sanitizing IC value as early as possible we avoid potential issues with using incorrect value. This is similar in spirit to what is done for MaxInterliveCount.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

if (VF.isScalar()) {
     if (ForceTargetMaxScalarInterleaveFactor.getNumOccurrences() > 0)
       MaxInterleaveCount = ForceTargetMaxScalarInterleaveFactor;
   } else {
     if (ForceTargetMaxVectorInterleaveFactor.getNumOccurrences() > 0)
       MaxInterleaveCount = ForceTargetMaxVectorInterleaveFactor;
   }
 
   // If trip count is known or estimated compile time constant, limit the
   // interleave count to be less than the trip count divided by VF.
   if (BestKnownTC) {
     MaxInterleaveCount =
         std::min(*BestKnownTC / VF.getKnownMinValue(), MaxInterleaveCount);
   }
 
   // If we did not calculate the cost for VF (because the user selected the VF)
   // then we calculate the cost of VF here.
   if (LoopCost == 0)
     LoopCost = expectedCost(VF).first;
 
   assert(LoopCost && "Non-zero loop cost expected");
 
   // Clamp the calculated IC to be between the 1 and the max interleave count
   // that the target and trip count allows.
   if (IC > MaxInterleaveCount)
     IC = MaxInterleaveCount;
-  else if (IC < 1)
+
+  if (IC < 1)
     IC = 1;
 
   // Interleave if we vectorized this loop and there is a reduction that could
   // benefit from interleaving.
   if (VF.isVector() && HasReductions) {
     LLVM_DEBUG(dbgs() << "LV: Interleaving because of reductions.\n");
     return IC;
   }

llvm/test/Transforms/LoopVectorize/SystemZ/zero_unroll.ll

+; RUN: opt -S -loop-vectorize -mtriple=s390x-linux-gnu -tiny-trip-count-interleave-threshold=4 -vectorizer-min-trip-count=8 < %s | FileCheck %s
+; RUN: opt -S -passes=loop-vectorize -mtriple=s390x-linux-gnu -tiny-trip-count-interleave-threshold=4 -vectorizer-min-trip-count=8 < %s | FileCheck %s
+
+define i32 @main(i32 %arg, i8** nocapture readnone %arg1) #0 {
+;CHECK: vector.body:
+entry:
+  %0 = alloca i8, align 1
+  br label %loop
+
+loop:
+  %storemerge.i.i = phi i8 [ 0, %entry ], [ %tmp12.i.i, %loop ]
+  store i8 %storemerge.i.i, i8* %0, align 2
+  %tmp8.i.i = icmp ult i8 %storemerge.i.i, 8
+  %tmp12.i.i = add nuw nsw i8 %storemerge.i.i, 1
+  br i1 %tmp8.i.i, label %loop, label %ret
+
+ret:
+  ret i32 0
+}
+
+attributes #0 = { "target-cpu"="z13" }
+