[PATCH] D42142: Adding Live-range reordering for Polly

[Siddharth via Phabricator via llvm-commits]

bollu added a comment.

Thanks a lot for the patch! I took a shot at a first round of review :) I can be nitpicky, so we can hash out the details as we go along with the review.



================
Comment at: lib/Transform/ScheduleOptimizer.cpp:391
+/// @see computeMustKillsInfo
+static bool isScalarUsesContainedInScop(const Scop &S,
+                                        const ScopArrayInfo *SAI) {
----------------
Can we refactor this stuff to prevent code duplication between this and `PPCGCodeGen`?


================
Comment at: lib/Transform/ScheduleOptimizer.cpp:411
+/// PPCG.
+static MustKillsInfo computeMustKillsInfo(const Scop &S) {
+  const isl::space ParamSpace = S.getParamSpace();
----------------
Can we pull this out as well to prevent code duplication?


================
Comment at: lib/Transform/ScheduleOptimizer.cpp:1923
+
+PPCGWrapperLRR ppcgObj;
+ppcg_scop *ps = NULL;
----------------
I am wary of global mutable state. Can we try to refactor this so that we don't need it? 


================
Comment at: lib/Transform/ScheduleOptimizer.cpp:2100
+  // NEW code starts here
+  if (LiveRangeReordering == false) {
+    DEBUG(dbgs() << "LiveRangeReordering == false\n");
----------------
Nit: prefer `if(!LiveRangeReordering)`


================
Comment at: lib/Transform/ScheduleOptimizer.cpp:2113
+    // This code is inspired from PPCGCodeGeneration::runOnScop()
+    ps = ppcgObj.createPPCGScop();
+
----------------
>From what I can tell, this object is a `ScopPass` to maintain the same interface? I would much rather it not be a class, and more importantly, **not a `ScopPass`**, because it's really not a pass :)

I'd also prefer it to not even exist. Rather, we can just have a function called `ppcg_scop createPPCGScop(const Scop &S)` or something like that.


================
Comment at: lib/Transform/ScheduleOptimizer.cpp:2162
+  if (!Schedule) {
+    FreePPCGObjects();
     return false;
----------------
This seems quite error prone. Can we use a `unique_ptr<ppcg_obj, (custom deallocator)>` to free this safely? 


Repository:
  rPLO Polly

https://reviews.llvm.org/D42142