[PATCH] D25095: [Polly] Ignore removed memory access. NFC.

[Michael Kruse via llvm-commits]

Meinersbur added a comment.

In https://reviews.llvm.org/D25095#558309, @grosser wrote:

> Now going back to this actual patch. I don't think it is necessary to today go full speed on these design changes, but am OK to continue removing full polly::MemoryAccesses for a while. Still, I think to make future changes easier it would be great if we could limit the generality of such changes to what is needed for DeLICM today. As said earlier, I understand that we cannot validate DeLICM retrospectively. But maybe we can check what kind of load/store memory accesses we allow to be removed.
>
> I went back to learn which memory accesses DeLICM removes today and it seems
>  delicm's cleanup routine does not drop loads at all and only drops stores that store a value that has been loaded before (so no div in between). It seems the load part of this patch can today not be 
>  reached, right? According to the comments in cleanup, you are planning to also remove the corresponding loads, but such loads would only be limited to the ones , where all uses are direct uses by store statements in the same basic block?Does your comment above refer to this situation, when mentioning that the vectorizer would crash if the load is not dropped? Or does this require more than one compute instruction between load and store?


the `cleanup()` currently implemented in DeLICM does the base minimum: Remove stores to array that a not necessary, but would impact the freedom of the scheduler. Removing loads as well would require something like a reference counter for loads (or just looking for remaining valid uses), which I might still implement. I think there were also situations were stores that are dead a left over, but I can't recall details at the moment. These are concerns of DeLICM, and I would be uncomfortable with adding these details to an unrelated pass such as CodeGen.

If the load is not dropped, it might read an location that has never been written to because all writes to it have been redirected, ie. and `Undef` value. Without the check added in this patch whether the loads has been dropped, it would probably crash because it required details from an deleted MemoryAccess. If there is a compute instruction in between in the same statement, then nothing should be removed in the first place.

> 
> 
>> Also consider that not generating a store is just as harmful for the generated code as replacing a load by an `Undef`. The missing store would be even harder to find.
>> 
>>> Also, if you commit this separately, which certainly would not hurt, a JSON test case would be amazing.
>> 
>> The json importer currently does not support removing `MemoryAccess`es. Implementing that will require much more code and certainly also its own review.
> 
> I suggested this as a way to obtain test coverage for this patch without the need for delicm yet. I would hope that JSON is easy to extend (e.g. just mark a memory access as 'deleted').
> 
> Another option would be to commit this change together with the delicm cleanup() routine after DELICM. We can then run delicm to test the codegen change and it also becomes more clear what
>  are the actual uses. I am in fact starting to prefer this approach slightly.

Committing with DeLICM would be fine with me, but doesn't solve the question of returning undef.

Do implicitly suggest to commit `cleanup()` separately from the main part?

Michael


https://reviews.llvm.org/D25095