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

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Michael,

here a first update. I would like to think a little bit more about this to get some illustrative examples (am busy on CC at the moment), but here already some comments on what I am thinking. (No need to change anything yet. I currently try to complete my understanding of this code).

In https://reviews.llvm.org/D24716#666792, @Meinersbur wrote:

> In https://reviews.llvm.org/D24716#666573, @grosser wrote:
>
> > >> 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
> >
> > Could such an example be added to the tests of isConflicting that illustrates why such a merge would result in an incorrect / unprecise answer.
>
>
> Could you suggest an `EXPECT(...)` line? I have no clue what kind of example you have in mind.


I am not sure how such test case can be constructed. You use "zones" to model occupied and undef sets in a knowledge. In isConflicting you always convert the zones to a set of timepoints (when comparing to writes). To me it looks as if we could as well store the zones as set of timepoints.  I am looking for an argument that explains why we need to store occupied and undef indeed as zones.



================
Comment at: lib/Transform/DeLICM.cpp:435
+      else
+        OS.indent(Indent) << "Unused:   <implicit>\n";
+      OS.indent(Indent) << "Written : " << Written << '\n';
----------------
Meinersbur wrote:
> 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.
>> 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

OK. The comments now make this very clear. Thank you!

>> Can you somehow print this, e.g. as "universe - Occupied".
> I don't understand this suggestion.

In "print(llvm::raw_ostream &OS, unsigned Indent = 0)" you just print "<implicit>" in case the corresponding set is a nullptr. Without reading the source code comments, I do not know what <implicit> means. Hence, this is difficult to understand. I wonder if we could print this in a way that one can easily understand what this means. E.g. by printing a string that shows how "implicit" is computed.

>> 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.

I wonder if there is an implicit assumption hidden. I need to think a little bit more about this to see if I can construct a test case that would validate this assumption and consequently result in incorrect answers being given by isConflicting. Overall, the correctness and the behavior of Knowledge should not depend on how Knowledge is used, but knowledge should have defined behavior for any input.

(In some way, we should make very clear which spaces a complement is actually formed. And how isConflicting works in case the sets are inconsistent -- if this can happen at all).


================
Comment at: lib/Transform/DeLICM.cpp:411
+  /// implicitly defined as the complement of #Occupied.
+  IslPtr<isl_union_set> Unused;
+
----------------
In your documentation and the code, you use "Unused" and "Undef" for the same thing. The set here is e.g., called Unused, but in the class documentation and the tests you talk about "Undef". Would it not be better to just use one of the two?


================
Comment at: lib/Transform/DeLICM.cpp:442
+    checkConsistency();
+  }
+
----------------
I wonder if it makes sense to document the implicit Occupied and Unused sets at this position and to explain precisely that the complement here means the complement for all spaces that are mentioned in the explicit of the two sets. In terms of interface, I could use see us enforcing and documenting that one of Occupied and Unused must be nullptr.


================
Comment at: lib/Transform/DeLICM.cpp:477
+
+    if (Unused && That.Occupied)
+      Unused =
----------------
This "if" is not needed. It is always implied by the asserts.


================
Comment at: lib/Transform/DeLICM.cpp:509
+    assert(Existing.Unused);
+    assert(Proposed.Occupied);
+
----------------
I tried to replace these lines by:

+    assert(Existing.Unused && !Existing.Occupied);
+    assert(!Proposed.Unused && Proposed.Occupied);

This seems to work. Would this be correct and could we make Knowledge consistently limited to this pattern? Would this make sense at all? Are there cases where isConflicting would return wrong results for certain values of Existing.Occupied or Proposed.Unused (which are not useful).


https://reviews.llvm.org/D24716