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

[Tobias Grosser via llvm-commits]

grosser added a comment.



In https://reviews.llvm.org/D25095#558989, @Meinersbur wrote:

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


OK. So it seems today (and in the immediate future) DeLICM will only drop stores, but loads are left untouched. Is this right?

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

Does this not mean that load memory accesses will never be removed by DeLICM and that consequently the "if (!Stmt.getArrayAccessOrNULLFor(Load))" is never false, due to DeLICM? If I add an assert before the "createUNDEF" none of the current DeLICM test cases fails, except the ones that already fail due to the missing "known" pass.

It seems as if this code currently is not needed. If this is the case, maybe we should postpone this discussion until we actually decide to remove loads in DeLICM?

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

Yes. The cleanup routine is certainly an independent change that would make sense to split off.

I personally prefer small changes similar to how Sven is looking for patch sets that are split up in independent changes. Optimally changes of commonly around 30-50 lines, with around 200 lines max. I understand that this takes additional work to split up patches, but in my experience it saves a lot of time in discussions. Obviously, if I am open to discuss different strategies to upstream such large patches.

(In general I prefer to avoid to build up such large patches in the first place, as working with them always adds a lot of overhead).


https://reviews.llvm.org/D25095