[PATCH] D43795: [New PM][IRCE] port of Inductive Range Check Elimination pass to the new pass manager

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D43795#1020066, @fedor.sergeev wrote:

> I'm not happy with BPI solution in this patch.
>  It is mainly to provoke discussions on how to solve this more gracefully, since no answers were given on llvm-dev@ :
>
>   http://lists.llvm.org/pipermail/llvm-dev/2018-January/120768.html
>   
>
> And yes, we want to have IRCE in new-pass-manager for our internal pipeline...


Sorry for delays getting to this. Generally the approach seems fine to me.

I would just suggest making BPI required as part of the loop standard analyses. It should be a separate patch (with clear discussion pointing out why it is useful), but BPI is useful to *many* loop passes I suspect, I just think we hadn't wired it up previously.

As part of this we should do some basic testing to make sure that the loop passes preserve BPI correctly.



================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:603
   Loop *createClonedLoopStructure(Loop *Original, Loop *Parent,
-                                  ValueToValueMapTy &VM);
+                                  ValueToValueMapTy &VM, bool isSubloop);
 
----------------
Use LLVM naming conventions?


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1770
+  InductiveRangeCheckElimination IRCE(AR.SE, *BPI, AR.DT, AR.LI);
+  auto LPMAddNewLoop = [&U](Loop *NL, bool subloop) {
+    if (!subloop)
----------------
Use LLVM-style naming?


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1791
+  InductiveRangeCheckElimination IRCE(SE, BPI, DT, LI);
+  auto LPMAddNewLoop = [&LPM](Loop *NL, bool) { LPM.addLoop(*NL); };
+  return IRCE.run(L, LPMAddNewLoop);
----------------
I'd use a commented out parameter name to make it more clear what is being ignored.


Repository:
  rL LLVM

https://reviews.llvm.org/D43795