[PATCH] D24716: [Polly] DeLICM/DePRE (WIP)

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Michael, another round of comments. Mostly trivial.


================
Comment at: lib/Transform/DeLICM.cpp:141
@@ +140,3 @@
+                cl::init(true), cl::cat(PollyCategory), cl::Hidden);
+
+cl::opt<unsigned long>
----------------
As discussed, these can be dropped.

================
Comment at: lib/Transform/DeLICM.cpp:183
@@ +182,3 @@
+  }
+};
+
----------------
Nice. If you want to submit this separately, you can probably use it to guard Transform/ScheduleOptimizer.cpp

================
Comment at: lib/Transform/DeLICM.cpp:343
@@ +342,3 @@
+/// The implementation currently returns the maximum number of dimensions
+/// encounters and 0 if none is encountered. However, most other code will most
+/// likely fail if this happens.
----------------
IT encounters

================
Comment at: lib/Transform/DeLICM.cpp:369
@@ +368,3 @@
+/// There is no such type like isl_union_space, hence we have to pass an
+/// isl_union_set over which to form the map.
+///
----------------
"which to form the map" -- Sounds grammatically wrong.

================
Comment at: lib/Transform/DeLICM.cpp:806
@@ +805,3 @@
+    //  assert(!isl_space_has_tuple_id(LifetimeDomainSpace, isl_dim_out));
+    // auto LifetimeZoneSpace = isl_space_range(LifetimeDomainSpace);
+  }
----------------
Dead code?

================
Comment at: lib/Transform/DeLICM.cpp:849
@@ +848,3 @@
+    // auto ThatUnknownDomain =
+    // getUnknownValInstDomain(isl_union_map_copy(That.Lifetime));
+    auto ThatKnowDomain = filterKnownValInst(That.Lifetime);
----------------
Dead code?

================
Comment at: lib/Transform/DeLICM.cpp:875
@@ +874,3 @@
+    // auto UnknownValUSet = isl_union_set_from_set(isl_set_universe(
+    // isl_space_set_from_params(isl_space_copy(ParamSpace))));
+
----------------
Dead code?

================
Comment at: lib/Transform/DeLICM.cpp:880
@@ +879,3 @@
+    // auto ThisUndefDomain =
+    // getUndefValInstDomain(isl_union_map_copy(This.Lifetime));
+    auto ThisKnown = filterKnownValInst(This.Lifetime);
----------------
Dead code?

================
Comment at: lib/Transform/DeLICM.cpp:886
@@ +885,3 @@
+    // auto ThatKnownOrUnknownDomain =
+    // isl_union_map_domain(isl_union_map_copy(That.Lifetime));
+    auto ThatUnknownDomain = getUnknownValInstDomain(That.Lifetime);
----------------
Dead code?

================
Comment at: lib/Transform/DeLICM.cpp:1398
@@ +1397,3 @@
+    return Acc1->getOriginalScopArrayInfo() == Acc2->getOriginalScopArrayInfo();
+  }
+
----------------
This is unused.

================
Comment at: lib/Transform/DeLICM.cpp:1413
@@ +1412,3 @@
+
+    return Acc1->getLatestScopArrayInfo() == Acc2->getLatestScopArrayInfo();
+  }
----------------
As discussed today, all but the last condition are unnecessary. Also, going through the code there seem to be five places that contain the following code:

```
        for (auto &PHIWriteStmt : *S) {
           for (auto *PHIWrite : PHIWriteStmt) {
             if (!isLatestAccessingSameScalar(NormMA, PHIWrite))
               continue;
```

I am slightly hesitant to ask you to refactor this completely, but outlining this in a function that just returns returns a vector of accesses certainly helps understandability here. It can then be used as

```
for (Accesses : getCorrespondingAccesses(PHIWrite))
  ....
```


================
Comment at: lib/Transform/DeLICM.cpp:1893
@@ +1892,3 @@
+    }
+  }
+
----------------
This function makes sense.

================
Comment at: lib/Transform/DeLICM.cpp:2067
@@ +2066,3 @@
+    // TODO: This does not consider uses of the scalar after the scop; we
+    // currently bail out if there such a use.
+    return Lifetime;
----------------
there IS such

================
Comment at: lib/Transform/DeLICM.cpp:2120
@@ +2119,3 @@
+
+  // TODO: No need fro RefMA
+  /// @param RefMA The MemoryAccess that is sought to be mapped to @p TargetElt
----------------
for

================
Comment at: lib/Transform/DeLICM.cpp:2528
@@ +2527,3 @@
+      if (MASize > StoreSize) {
+        DEBUG(dbgs() << "    Reject because storage size is not sufficient.\n");
+        continue;
----------------
insufficient

================
Comment at: lib/Transform/DeLICM.cpp:2729
@@ +2728,3 @@
+                       << " pruned because it writes only a single element\n");
+          continue;
+        }
----------------
I would suggest to always skip these two for now. This clearly gives us less test coverage, but for this patch this is good as it ensures that we do not need any special handling for these corner cases.


https://reviews.llvm.org/D24716