[PATCH] D32026: [Polly][DeLICM] Use Known information when comparing Occupied and Written. NFC.

[Michael Kruse via Phabricator via llvm-commits]

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


================
Comment at: lib/Transform/DeLICM.cpp:684
+    // Proposed.Unused (they never conflict) and then see whether the written
+    // value is already in Proposed.Known.
+    //
----------------
grosser wrote:
> This comment does not seem to be in line with what actually happens.  Where do we:
> 
>  - "remove the writes that write into Proposed.Unused"
>  - "see wheather the written value is already in Proposed.Known"
> 
> It seems what we do is:
> 
>   - Intersect the Existing Writes with the Currently Occupied values to find the set of possibly conflicting writes.
> 
> - See if the set of possibly conflicting writes only contains known values by checking if it is a subset of ProposedFixedKnown.
> 
> 
In my understanding this is exactly what the code below does:

> "remove the writes that write into Proposed.Unused"
```
auto ExistingConflictingWrites = Existing.Written.intersect_domain(ProposedFixedDefs);
```
ProposedFixedDefs = Proposed.Occupied, i.e. the intersect removes Proposed.Unused.

> "see wheather the written value is already in Proposed.Known"
```
auto SomeWrittenVal = ProposedFixedKnown.intersect(ExistingConflictingWrites);
```
SomeWrittenVal will contain matched pairs where the value written is the same as the value already known. The final condition is whether all writes overlapping with Proposed.Occupied has a match.


================
Comment at: lib/Transform/DeLICM.cpp:707
+
+    if (!ExistingConflictingWritesDomain.is_subset(SomeWrittenValDomain)) {
       if (OS) {
----------------
grosser wrote:
> This seems more complex as needed. For me the following code also works:
> 
> ```
>     auto ProposedFixedDefs =                                                     
>         convertZoneToTimepoints(Proposed.Occupied, true, false);                 
>     auto ProposedFixedKnown =                                                    
>         convertZoneToTimepoints(Proposed.Known, isl::dim::in, true, false);      
>     auto ExistingConflictingWrites =                                             
>         Existing.Written.intersect_domain(ProposedFixedDefs);                    
>                                                                                  
>     if (!ExistingConflictingWrites.is_subset(ProposedFixedKnown)) {   
> ```
> 
> Some of the later code can -- if needed at all -- be sunk into the "if (OS)".
> 
> Or did I miss something?
Added a test case where the difference between your implementation and mine matters.

We could simplify to your version if we assume that Existing.Written contains at most one value (`assert(isl_union_map_is_single_valued(Existing.Written))`). I want to leave the possibility open to properly handle PHINodes.


================
Comment at: lib/Transform/DeLICM.cpp:738
+
+    if (!ProposedWrittenDomain.is_subset(ProposedWrittenSomeValDomain)) {
       if (OS) {
----------------
grosser wrote:
> AFAIU this is equivalent to the following code:
> 
> ```
>     auto ProposedWrittenDomain = Proposed.Written.domain();                      
>     auto ExistingAvailableDefs =                                                 
>         convertZoneToTimepoints(Existing.Unused, true, false);                   
>     auto KnownIdentical = Existing.Known.intersect(Proposed.Written);            
>     if (!ProposedWrittenDomain.is_subset(                                        
>             ExistingAvailableDefs.unite(KnownIdentical.domain()))) {  
> 
> ```
> 
> which is _very_ similar to the earlier code, a lot more concise, and does not even need an expensive subtract. I just now realized that we can avoid the subtract by computing the union of the KnownIdenticalSet with the ExistingAvailableDefs, rather than subtracting it beforehand. The same appraoch likely also applies to D32025.
Both implementations do the same. Mine has the union the intersection and adds the unknown value to both arguments such that it survives the intersection. This can indeed be made simpler by doing the union afterwards. I opted to your version (There was a`convertZoneToTimepoints` call missing and I introduced an intermediate result variable `IdenticalOrUnused`).


https://reviews.llvm.org/D32026