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

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Hi Michael,

please ignore the previous email. It got sent incomplete.

First, I like the changes to computeReachingWrite. The code duplication is nicely removed. Very cool!

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

I also agree with you that keeping the additional cases for now makes sense -- especially as we have good test coverage. In fact, I like the idea of adding this functionality to ISLTools.cpp. It clearly is self-contained, well documented, and well tested. Please feel free to push these out already.

> 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

Could such an example be added to the tests of isConflicting that illustrates why such a merge would result in an incorrect / unprecise answer.

>> 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%2

I did not mean that we should merge Unused/Occupied. I agree that having both makes a lot of sense. What I was suggesting above was if we should prohibit that both can be set simultaniously. The reason for this is that we now need to always support an explicit and an implicit representation of these sets. As such I always need to convince myself that both code paths are correct (or that ignoring one is correct). In the actual code that we run, we always seem to use implicit sets. So I wonder why we do not just limit our implementation to implicit sets and also test it with implicit sets.


https://reviews.llvm.org/D24716