[PATCH] D15687: [Polly] Add conditions for unnecessary value reads

[Michael Kruse via llvm-commits]

Meinersbur added a comment.

In http://reviews.llvm.org/D15687#336428, @grosser wrote:

> Hi Michael,
>
> in this patch only the isSynthesisable change seems to actually affect the output, everything else appears to not yet affect anything. 
>  My feeling is that some of the statements added here will be removed in your later patch at a different location. I would prefer to keep them in the same patch to better see that they are just moved around.


As a reminder, I extracted out this patch out of http://reviews.llvm.org/D15706 on your request. Nothing will be removed again, it is even here because of the later patch.

> Regarding the isSynthesable change itself. It is touching an area that is a little fishy.


This and http://reviews.llvm.org/D15706 are intended to reduce the fishyness. This is a good thing, isn't it?

> Specifically, it will disable (all?) integer read-only memory access modeling as most (all?)l read-only memory accesses are synthesis-able. I need to think about this a little bit to understand if/what that means, to which extend we do this today and how we can correctly model read-only memory accesses then.


The idea is simple: Everything that is synthesizable (depending only on SCoP parameters and induction variables) can be inserted directly into the code and therefore does not require reading anything. You are right, since integer definitions defined before the scop are added as parameter, there is no reason to model them as accesses. Why should it? Why model those as parameter _and_ pass them using allocas? This is to my understanding the intention from the beginning. If not, please tell me the rules when read-only accesses are needed.

This should also exactly be what's required for OpenMP kernels. The SCoP parameters need to be passed to the kernel in any case because there might be (other) synthesizable SCEVs/isl_pw_aff in there that depend on these params. Passing a parameter a second time via a kernel function argument is redundant.

> Is the isSynthesisable part of this patch blocking subsequent patches?


Yes, blocks http://reviews.llvm.org/D15706 and http://reviews.llvm.org/D12975; I am unable to understand the idea of read-only accesses in the current implementation. There seems to be no consistency. Eg. there is a difference whether it is used by an MK_Value, MK_PHI, or MK_PHI incoming value that is defined in a different statement. http://reviews.llvm.org/D15706 and http://reviews.llvm.org/D12975 modify the creation of MemoryAccesses and I cannot change it accurately if I can't even tell whether it is a bug or by design.


http://reviews.llvm.org/D15687