[PATCH] D62989: [Unroll] Do NOT unroll a loop with small runtime upperbound

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:795
+  if (!(UP.UpperBound || MaxOrZero) || UnrollByMaxCount > UnrollMaxUpperBound)
+    UnrollByMaxCount = 0;
+
----------------
zzheng wrote:
> hfinkel wrote:
> > Did you make this new variable so that you could apply this `!(UP.UpperBound || MaxOrZero) || UnrollByMaxCount > UnrollMaxUpperBound` condition only to full-unrolling case and not to the partial unrolling case?
> No, new variable UnrollByMaxCount replaces use of MaxTripCount in full-unrolling case.
> 
> We only full-unroll a runtime loop by its upper bound if MaxOrZero is true; otherwise runtime unrolling will handle it, sometimes unroll more than MaxTripCount. Runtime unrolling needs to know MaxTripCount, so we cannot set it to zero here.
> 
> https://reviews.llvm.org/D63064 is trying to solve the same problem for ARM target.
> 
> I'm confuse why this relevant to partial unrolling, which only applies to known TripCount. We only calculate MaxTripCount and MaxOrZero if TripCount is 0.
> I'm confuse why this relevant to partial unrolling, which only applies to known TripCount. We only calculate MaxTripCount and MaxOrZero if TripCount is 0.

I asked because the following conditional is added in this patch (before the pre-existing debug output), which seems to apply MaxTripCount to limit partial unrolling (at least if the debug output below the conditional is accurate). Maybe the debug output should say runtime unrolling?


```
  if (MaxTripCount && UP.Count > MaxTripCount)
    UP.Count = MaxTripCount;

  LLVM_DEBUG(dbgs() << "  partially unrolling with count: " << UP.Count
                    << "\n");
```

> new variable UnrollByMaxCount replaces use of MaxTripCount in full-unrolling case.

Okay. If UnrollByMaxCount is only used during full unrolling, then can we name it FullUnrollMaxCount, or similar, to make that clear?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62989