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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur marked 43 inline comments as done.
Meinersbur added a comment.

Thanks for the review

In https://reviews.llvm.org/D24716#665836, @grosser wrote:

> I also noted that you provide implementations for all corner cases always including or not including bounds. On the other side, most of the actual code is currently not using any of these (there is commonly only one configuration used). As this all looks very well tested, I don't want to ask you to drop all the special cases, especially as I have the impression that these may be useful for some of the later code or later discussions on how to model certain things best. Hence, I am fine leaving them in. However, if you have any cases in mind that are certainly not needed any more, don't hesitate to drop them.


I feel relative strongly about keeping them. They were certainly helpful in development when I did not yet have a singular definition of what a "zone" is. Currently a zone 0 < 1 < 1 is consistently represented by the integer set `{ [1] }`. There is no strict necessity to fo this everywhere. E.g. it might be more intuitive to use the start of the range instead. These computations are currently agnostic to what a zone is (could be moved to ISLTools.cpp as well maybe?) and I would perceive it as a loss when they suddenly would.

In https://reviews.llvm.org/D24716#666155, @grosser wrote:

> I am not sure if Knowledges must be implicit or if also both sets can be explicit. In isConflicting you state "current implementation requires it to be implicit", but when testing you seem to compute two explicit sets using completeLifetime. This seems inconsistent.


That is comment was outdated. In previous revisions Unused, Known and Unknown was stored in the same map (instead in one `Unsued` and one `Occupied`). Two of them had to be defined, the third was implicit, hence "required" to be implicit.

> Do you have an example that makes clear why you need zones, and cannot just track the values / life ranges? That seems like an interesting test for isConflicting.

See file comment in `DeLICM.cpp`

> Assuming both implicit and explicit representation are supported. I wonder if one of them would be enough? Or is there a reason to keep both? I have the feeling that having both currently makes the code (unnecessarily) hard to read?

It's just the opposite, it makes it clearer that Unused/Occupied can be `nullptr` and therefore easier to understand. Previous versions of this patch used a flag which one is implicit, hence also requiring that one of the is implicit. I invite you to look into these versions to see if whether they are easier to understand.

The sets can be `nullptr` only because we want avoid computing them, not because of something conceptional. If they are still given, they are ignored. This is fine for the unit tests which are small, but the complement operation blew up easily in my experiments with even small programs.

Michael



================
Comment at: lib/Transform/DeLICM.cpp:435
+      else
+        OS.indent(Indent) << "Unused:   <implicit>\n";
+      OS.indent(Indent) << "Written : " << Written << '\n';
----------------
grosser wrote:
> What does it mean to be implicitly unused. Can you somehow print this, e.g. as "universe - Occupied".
> 
> What happens if the first set does not contain elements from each space? Assuming we never write to a given array? This would mean it does not appear in Occupied and is completely unused. Now, the universe we compute won't contain an element from this space, after the (implicit) subtraction it won't appear, right?
> 
> You seem to be working around this in the tests, but it is not clear how this is resolved in general or which preconditions need to be satisfied to make this work.
> What does it mean to be implicitly unused. 

The unused set has not been computed explicitly, but is assumed to be the complement of `Occupied`

> Can you somehow print this, e.g. as "universe - Occupied".

I don't understand this suggestion.

> What happens if the first set does not contain elements from each space?

Does not happen. In ZoneAlgorithm one of the two sets will always be implicit. In DeLICMTests `unionSpace()` is used.

> Assuming we never write to a given array? This would mean it does not appear in Occupied and is completely unused. Now, the universe we compute won't contain an element from this space, after the (implicit) subtraction it won't appear, right?

As there is no write to the array, `greedyCollapse()` will not try to map anything to it.


================
Comment at: lib/Transform/DeLICM.cpp:621
+  /// violate the first condition. There is also at most one write to a scalar,
+  /// satisfying the second condition.
+  bool isCompatibleStmt(ScopStmt *Stmt) {
----------------
grosser wrote:
> Is this true? What about region statements that write to a PHI node?
True since commit r258809: Unique phi write accesses


================
Comment at: lib/Transform/DeLICM.cpp:638
+        if (!isl_union_map_is_disjoint(Stores.keep(), AccRel.keep()))
+          return false;
+
----------------
grosser wrote:
> Maybe add a test case?
You previously asked me to remove the non-"After Accesses" parts from the test cases. This currently cannot be tested effectively without more diagnostics.

There would be the possibility to copy reduction_preheader.ll a few times and add variations. In most cases the variations would already cause it not to be mapped (longer lifetimes), not because the SCoP is incompatible (Test would succeed even if this check was removed)

So I could add loads/store combinations to other array elements than the ones necessary for at least one mapping. But in the mid term I'd only want to mark the elements as incompatible, not the entire SCoP. That would then require to rewrite all tests.

Originally I intended to more systematically test all combinations of loads and stores orders in one ScopStmt when more diagnostics is available. I'd add that separately sometime later.


================
Comment at: lib/Transform/DeLICM.cpp:647
+          return false;
+        }
+
----------------
grosser wrote:
> What would be such a case? Is there an easy opportunity for a test case?
memcpy, memmove, memset

About testing, see comment above


================
Comment at: lib/Transform/DeLICM.cpp:653
+            !isl_union_map_is_disjoint(Loads.keep(), AccRel.keep()))
+          return false;
+
----------------
grosser wrote:
> Should we have a test case for this? (including a comment in the test case that states that this is the reason it is not DeLICMed)
About testing, see comment above


================
Comment at: lib/Transform/DeLICM.cpp:655
+
+        // Do not allow more than one store to the same location.
+        if (!isl_union_map_is_disjoint(Stores.keep(), AccRel.keep()))
----------------
grosser wrote:
> Should we have a test case for this? (including a comment in the test case that states that this is the reason it is not DeLICMed.
About testing, see comment above


================
Comment at: lib/Transform/DeLICM.cpp:1489
+          continue;
+        }
+
----------------
grosser wrote:
> The previous four lines can be dropped, but changing !MA->isWrite() to !MA->isMustWrite() a couple of lines earlier.
That wouldn't print a debug message.


================
Comment at: lib/Transform/DeLICM.cpp:1761
+  Knowledge Proposed(std::move(ProposedOccupied), std::move(ProposedUnused),
+                     std::move(ProposedWrites));
+
----------------
grosser wrote:
> Why do we move this stuff here? This does not seem to be performance sensitive.
This is to avoid to exposing `Knowledge` only for testing `isConflicting`.


================
Comment at: unittests/DeLICM/DeLICMTest.cpp:379
+  completeLifetime(Universe, ExistingOccupied, ExistingUndef);
+  completeLifetime(Universe, ProposedOccupied, ProposedUndef);
+
----------------
grosser wrote:
> Why do you compute the universe here? I was under the impression that Knowledges can also be created with implicit Unkown, Unused sets and should work well with them. In fact, there is a comment in isConflicting that claims that it _only_ works with implicit sets?
> 
> Would the tests still work if you do not create explicit representations of Unknown and Unused?
> Why do you compute the universe here?

To compute an argument that got only a `nullptr`.

> I was under the impression that Knowledges can also be created with implicit Unkown, Unused sets and should work well with them. In fact, there is a comment in isConflicting that claims that it _only_ works with implicit sets?

See main comment.

> Would the tests still work if you do not create explicit representations of Unknown and Unused?

Yes, but the explicit representation is ignored.


https://reviews.llvm.org/D24716