[PATCH] D58770: [LSR] Attempt to increase the accuracy of LSR's setup cost

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

> The Hexagon related test changes look good to me. Neither of these test changes indicate that your patch causes any problems. In swp-carried-1.ll, the hardware loop is no longer generated. In this case, I should just change the test to use a real value for the initial loop induction variable, %v4, instead of undef, say 0 (I can do that later). In swp-epilog-phi5.ll, the compiler is now generating an extra, innermost, hardware loop, so that's why the test changes from loop0 to loop1. The loop1/endloop1 instructions represent an outer hardware loop. This is another test that I should fix since subtle changes in the generated code cause the compiler to create/not create a hardware loop.

Thanks. I didn't realise there could be nested hardware loops. Good to see there are not worse. Are you OK with me leaving them as they are here, and you fixing them as you think is best? I had a go at fixing swp-carried-1.ll, but wasn't sure how best to keep testing the old behaviour without the -lsr-recursive-setupcost=0 option, even after removing the undefs. It was using a scev like {(128 + (-1 * undef)),+,-1} before.

> Is the lsr-setupcost test new? I don't see it in my working directory.

Yep, that's not is tree yet. I was trying to show the differences caused by the patch. (It would have been clearer if I'd have explained that.)



================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:1224
+    return getSetupCost(S->getStart());
+  if (!EnableRecursiveSetupCost)
+    return 0;
----------------
samparker wrote:
> Shouldn't we have this before the first recursive call?
It's was trying to replicate the behaviour from before this patch, so allowing the recursion into the addrec. EnableRecursiveSetupCost may not have been the best name for that exactly. I'll update the code to match the name better.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:1303
   // instructions in the preheader.
-  if (!isa<SCEVUnknown>(Reg) &&
-      !isa<SCEVConstant>(Reg) &&
-      !(isa<SCEVAddRecExpr>(Reg) &&
-        (isa<SCEVUnknown>(cast<SCEVAddRecExpr>(Reg)->getStart()) ||
-         isa<SCEVConstant>(cast<SCEVAddRecExpr>(Reg)->getStart()))))
-    ++C.SetupCost;
+  C.SetupCost += getSetupCost(Reg);
 
----------------
samparker wrote:
> Use a lamda instead?
It needs to be recursive (and recursive into the lambda in std::accumulate). I got tired of trying to make that work and pulled it out into a separate function.


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

https://reviews.llvm.org/D58770