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

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Michael,

some of the comments from our discussion.

Best,
Tobias



================
Comment at: include/polly/DeLICM.h:36
+                                              bool IncludeZoneStart,
+                                              bool IncludeZoneEnd);
+
----------------
Add comment?


================
Comment at: lib/Transform/DeLICM.cpp:40
+//
+// Therefore, an integer i in the set represents the zone ]i-1,i[, ie. strictly
+// speaking the integer points never belong to the zone. However, depending an
----------------
grosser wrote:
> i.e., strictly speaking
Comma after i.e., missing.


================
Comment at: lib/Transform/DeLICM.cpp:359
+/// isl_union_set_extract_set() on the other hand does not check whether there
+/// is (at most) one isl_set in the union, ie. how it has been constructed is
+/// probably wrong.
----------------
grosser wrote:
> i.e.,
Comma after i.e., missing.


================
Comment at: lib/Transform/DeLICM.cpp:654
+void simplify(IslPtr<isl_set> &Set) {
+  Set = give(isl_set_compute_divs(Set.take()));
+  Set = give(isl_set_detect_equalities(Set.take()));
----------------
Meinersbur wrote:
> grosser wrote:
> > compute_divs can sometimes make a map a lot more complex. Did you had a case where this is necessary.
> > 
> > We also should ask sven at some point if he can provide a simplify function for isl that does all simplifications in the right order.
> I am not aware that this can make sets more complex, it just provides an additional explicit representation of a div that should make follow-up computations faster and was sometimes necessary for coalescing.
> 
> According to the source comments the issue is rather that compute_divs can be an expensive operation, but so is coalescing. If possible I'd not call `simplify()` at all but that results in other operations taking much longer, e.g. `lexmin`.
OK. We probably need to evaluate this in practice. Let's start with what you have.


================
Comment at: lib/Transform/DeLICM.cpp:1080
+  ///
+  /// The domain of the result will as narrow as possible.
+  IslPtr<isl_map> getScatterFor(ScopStmt *Stmt) const {
----------------
grosser wrote:
> IS as
IS BE?? Just 'is as'?


================
Comment at: lib/Transform/DeLICM.cpp:83
+// This is why most of the time isl_union_map has to be used.
+//
 //===----------------------------------------------------------------------===//
----------------
Maybe give an overview comment describing the high-level structure of this approach.


================
Comment at: lib/Transform/DeLICM.cpp:477
+  return Result;
+}
+
----------------
Several of these functions above are not really tied to your DeLICM approach, but are generally useful. I would suggest to add these separately in an ISLTools.cpp file, in the optimal case with separate unit tests.

You can probably already upstream these today.


================
Comment at: lib/Transform/DeLICM.cpp:689
+  return isl_set_is_singleton(Set.keep());
+}
+
----------------
Pull in getAccessRelation.


================
Comment at: lib/Transform/DeLICM.cpp:923
+  /// isl_union_set.
+  IslPtr<isl_union_set> EmptyUnionSet;
+
----------------
Drop this one as well?


================
Comment at: lib/Transform/DeLICM.cpp:1092
+    return getDomainFor(MA->getStatement());
+  }
+
----------------
The above all really seem to be belong into ScopInfo. Let's leave them here for know, as this makes updating this patch easier, but maybe we can keep in mind that we want to unify this at some point.


================
Comment at: lib/Transform/DeLICM.cpp:1845
+    auto Result =
+        give(isl_union_map_wrap(isl_union_map_reverse(WriteTimepoints.copy())));
+
----------------
I believe we should not use auto so much. As you correctly pointed out, auto is suggested by LLVM to be only used in rare cases. We had in Barcelone a discussion how much we should use auto and it seems we choose a rather liberal direction. I now believe that following LLVM here closer is better.

Now, currently auto makes sense as it saves the long IslPTr<.....> type so you probably do not need to change anything here immediately. We should just keep this in mind, when potentially switching to the C++ bindings.


================
Comment at: lib/Transform/DeLICM.cpp:1791
+      }
+    }
+
----------------
Meinersbur wrote:
> grosser wrote:
> > Any reason you do not use scop->getReads(), scop->getWrites(), scop->getMayWrites()?
> > 
> > One might claim that your code prevents us from scanning the scop four times for different kinds of accesses, but this is certainly not a large performance difference (if measurable at all
> > given the later more costly computations). If we want to add such a specialization, I would prefer to add this after the initial delicm implementation as a well-benchmarked and consequently justified performance optimization.
> `getReads()` etc. do also return scalar accesses, ie. would need to iterate over the result again to filter those out. `addArrayWriteAccess` also computes the ValInsts for which we'd also need to iterate over the accesses.
> 
> I could remove the `addArrayReadAccess()` part if `scop->getAccessesOfType()` was public.
OK. I think this functionality should at some point be merged with ScopInfo, but let's leave it here for know to reduce the dependences to ScopInfo.


================
Comment at: unittests/DeLICM/DeLICMTest.cpp:423
+                                  "{}", "{}", nullptr, "{}"));
+
+  EXPECT_FALSE(checkIsConflicting("{}", nullptr, "{}", "{ Dom[0] -> [] }", "{}",
----------------
To fix formatting, maybe use:

```
struct {
  type *Known, type *undef, type *written, type *unkown
} input;

void foo() {
  EXPECT_FALSE(conflicts(
      {aasdfsadf, basdfsadf, casdfasf, dsadfsadfsadfdsafdsafsafdfasdfsadf},
      {a, basdfsafd, casdfdsaf, dsadfsadf}));
}
```


================
Comment at: unittests/DeLICM/DeLICMTest.cpp:426
+                                  "{}", nullptr, "{}"));
+
+  EXPECT_FALSE(checkIsConflicting("{ Dom[i] -> Val[] : 0 < i  }", nullptr,
----------------
It also seems as if these tests still cover the Known part. It would be nice if we could have a set of simpler tests that just take the current simpler isConflicting implementation and do not go through the complexity of checkIsConflicting.


https://reviews.llvm.org/D24716