[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:
> 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.




http://reviews.llvm.org/D15559