[PATCH] D15559: [LoopUnrollRuntime] Do unroll when IV's setp is one or minus one

[Junmo Park via llvm-commits]

flyingforyou added inline comments.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:325-331
@@ +324,9 @@
+                  SE->getSCEV(ExitCondICmp->getOperand(0)), L))) {
+        if (AR->isAffine() && AR->getOperand(1)->getSCEVType() == scConstant) {
+          if (const SCEVConstant *SCConst =
+                  cast<SCEVConstant>(AR->getOperand(1))) {
+            if (SCConst->getValue()->isOne() ||
+                SCConst->getValue()->isMinusOne())
+              AllowExpensiveTripCount = true;
+          }
+        }
----------------
mzolotukhin wrote:
> flyingforyou wrote:
> > mzolotukhin wrote:
> > > There is a method `getStepRecurrence` in `AR`, which you might want to use here.
> > > 
> > > Also, I don't think that changing `AllowExpensiveTripCount` here is a good way to achieve what you try to achieve. Why (and where) is this tripcount considered expensive? To me it looks like we should rather fix those places to recognize cases like this as 'inexpensive'.
> > > There is a method getStepRecurrence in AR, which you might want to use here.
> > 
> > Thanks, I will change this later.
> > 
> > 
> > > Why (and where) is this tripcount considered expensive?
> > When we run test case "high-cost-trip-count-computation.ll", we can get expression of Tripcount SCEV likes below.
> > test1 func: (1 + ((-1 + (-1 * %v12) + (8193 smax (%v12 + %step))) /u %step))
> > test2 func: (1 umax ((23 + %conv7) /u %conv7))
> > 
> > test1's division is generated for computing tripcount. Because %step is unknown. This case, we consider this is expensive. We need to generate new division for computing tripcount.
> > test2's division is generated by original code which is in loop pre header(entry). In this case, isHighCostExpansion can't figure it out this is for computing trip count or it came from original code.
> > And isHighCostExpansion is also used by IndVarSimplify. It's not only for LoopUnrollRuntime.
> > 
> > If you still think changing AllowExpensiveTripCount is not good way, please let me know your idea.
> > 
> > Junmo.
> > 
> > 
> Ok, I think I understood the issue.
> 
> So, in `isHighCostExpansionHelper` routine there is a check
> ```
> if (At && findExistingExpansion(S, At, L) != nullptr)
>     return false;
> ```
> 
> Ideally, it should find the existing computation and immediately report that the expansion isn't expensive. In practice, this function doesn't always work as we'd like it to work (see e.g. https://llvm.org/bugs/show_bug.cgi?id=24920), but I'd suggest fixing it instead of working around in other places.
Thanks for sharing bug about "isHighCostExpansionHelper" function.

LoopUnrollRuntime use isHighCostExpansion function without "At". This means it only want to check TripCountSC has expensive expression or not.

So, checking IV's step may not be working around for LoopUnrollRuntime.

Junmo.

================
Comment at: test/Transforms/LoopUnroll/high-cost-trip-count-computation.ll:45-49
@@ +44,7 @@
+  br label %for.body
+for.body:
+  %rem1 = phi i64 [ %rem0, %entry ], [ %rem2, %for.body ]
+  %tmp1 = phi float [ 1.000000e+00, %entry ], [ %conv21, %for.body ]
+  %sum1 = phi float [ 0.000000e+00, %entry ], [ %add18, %for.body ]
+  %k1 = phi i64 [ %div12, %entry ], [ %dec, %for.body ]
+  %mul1 = mul i64 %rem1, 48271
----------------
mzolotukhin wrote:
> It's definitely possible to simplify this testcase by removing lots of unneeded code from this loop. Please keep only parts that are needed to expose the issue.
Yes. You're right. I will remove unnecessary codes.
Thanks.


http://reviews.llvm.org/D15559