[PATCH] D32025: [Polly][DeLICM] Use Known information when comparing Existing.Occupied and Proposed.Occupied. NFC.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur marked 8 inline comments as done.
Meinersbur added inline comments.


================
Comment at: lib/Transform/DeLICM.cpp:381
+///
+/// @return { Domain[] -> ValInst[] }
+isl::union_map makeUnknownForDomain(isl::union_set Domain) {
----------------
grosser wrote:
> Nice. Can you possibly add a comment that repeats the information that "Written values that cannot be identified are represented by an unknown  ValInst[] (an unnamed tuple of 0 dimension). It conflicts with itself. " and add a comment that more details can be found in the documentation for known?
> 
Note that a more detailed explanation of what "ValInst" means is part of the `makeValInst` documentation in D31247.

I expanded the comment here anyway.


================
Comment at: lib/Transform/DeLICM.cpp:635
+    // In order to not conflict, one of the following conditions must apply for
+    // each element/lifetime interval:
+    //
----------------
grosser wrote:
> I was confused why you wrote "element  / lifetime interval", as I did not understand if these were identical terms for the same thing or different things. Maybe you can clarify in half a sentence what element and lifetime mean here. Maybe something like:
> 
> "In order to not conflict, one of the following conditions must apply for each element or each lifetime. (We reason about single elements in case ... and about lifetimes in case ...)"
The domain has the form ```[Element[] -> Zone[]]``` which I tried to express as "element / lifetime interval". I.e. the cross product, not the union as I would understand from your suggestion.

I'll try to reformulate.


================
Comment at: lib/Transform/DeLICM.cpp:641
+    //
+    // 2. Both contain the same value.
+    //
----------------
grosser wrote:
> I am surprised you formulate this as symmetric condition. The code below does not seem to be symmetric, as it does not even look at Proposed.Unused. Meaning, it would not return true if a value is in Existing.Occupied and not in Proposed.Unused. For me, it would probably have been easier to understand if the comment would be formulated closer to the asymmetric check the code performs.
> 
> E.g., something like the following:
> 
> ```
> In order to not conflict, one of the following conditions must apply:
> 
> 1. If occupied in Proposed, it is unused in Existing
> 
> 2. If occupied in Proposed and occupied in Existing, the Known value in proposed must match the known value in Existing.
> ```
> 
> 
> If the behavior is indeed symmetric, it might make sense to mention this afterwards and explain why the asymmetric code indeed results in symmetric behavior.
> 
The conceptional behavior of `isConflicting` is indeed symmetric, as tested by DeLICMTests.

Asymmetry comes in because "Existing" defines occupied, but "Proposed" defines unused, which was done to avoid a complement operation.

The comment first describes what the code intends to do (which is as mentioned symmetric) and then how this behavior was implemented.

I'd be very confused if it was the other way around. How can I understand an implementation without knowing what it was supposed to do? That could be called reverse engineering.


================
Comment at: lib/Transform/DeLICM.cpp:645
+    // checking the conditions separately, we check only the second conditions
+    // and ensure that it will also apply when then first condition is true.
+    // This is done by adding another "the value occupying Proposed" to
----------------
grosser wrote:
> conditions -> condition
> will also apply -> also applies
> 
> I did not understand what you mean by "Instead of partitioning the lifetimes into a part occupied by both". I would probably have calculated this as:
> 
> ```
> +    auto KnownIdentical = Proposed.Known.intersect(Existing.Known).domain();
> +    auto ProposedOccupied = Proposed.Occupied.subtract(KnownIdentical);
> +    if (!ProposedOccupied.is_subset(Existing.Unused)) {
> ```
> 
> This has the benefit that we do not affect compile time for cases where no Known information is available. Also, this 'unknown' stuff does not need to be explained.  Was there a reason you did not take this approach?
> 
> If you found cases where such a subtract would become too expensive, it would be good to have a such a test case committed and describe both the simple and the more complex solution as well as the reason why the more complex solution was taken. 
Your code does exactly what I described in "instead of ..." part. I added a test case where the subtract would blow up.

If `Known` is empty, the unites just return the Unused/Occupied set. No additional cost.


================
Comment at: lib/Transform/DeLICM.cpp:650
+    // purpose such that we can re-use Existing.Unused to also match unknown
+    // values in Proposed.Written in a later check.
+    auto ProposedOccupiedAnyVal = makeUnknownForDomain(Proposed.Occupied);
----------------
grosser wrote:
> Has this later check already been committed? Are you indeed referring to a later commit, or just to the line where SomeVal is computed?
removed


================
Comment at: lib/Transform/DeLICM.cpp:657
+
+    auto SomeVal = ExistingValues.intersect(ProposedValues);
+    auto SomeValDomain = SomeVal.domain();
----------------
grosser wrote:
> What does SomeVal mean?
Renamed to `MatchingVals`


================
Comment at: unittests/DeLICM/DeLICMTest.cpp:244
+                                       {nullptr, "{ Dom[0] }", "{}"}));
+
   // Check occupied vs. written.
----------------
grosser wrote:
> Can you add test cases:
> 
> ```
> EXPECT_TRUE(checkIsConflictingKnown(
>    {"{ Dom[i] -> [] }", nullptr, "{}"},
>    {"{ Dom[i] -> [] }", nullptr, "{}"}
> ));
> ```
> 
This is identical to
```
  EXPECT_TRUE(checkIsConflicting({"{ Dom[i] }", nullptr, "{}"},
                                 {"{ Dom[i] }", nullptr, "{}"}));
```
at line 118.


https://reviews.llvm.org/D32025