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

[Tobias Grosser via llvm-commits]

grosser added a comment.

In http://reviews.llvm.org/D15687#336706, @Meinersbur wrote:

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


Sure. I remember that I asked for it. We just need to make sure the patches are split in a way that everything is tested, otherwise it is difficult to distinguish between dead code, code that is used and tested later and code for which we do not add more test coverage.

In this very patch it seems only isSynthesisable is tested. All other additions can be removed without any test failures. My feeling is that they current code will never call ensureValueRead with arguments that trigger the code that can be removed. If this is indeed the case, these changes (with the exception of isSynthesisable) can not be split off from the next patch. In case they can be reached even today, it would be good to add appropriate test cases.

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


Just to explain the fishiness a little bit. The issue is that we only introduce parameters for SCEVs that are part of loop bounds or array indexes, but not for non-affine index expressions or scalar dependences that are e.g. just compute a value that is stored to memory. We do not introduce parameters for two reasons: First, we do not need them to model iteration domains, so not adding them keeps the dimensionality of the integer sets low. Second, we can not identify parameters the way we do this for affine expressions as some of the scalar evolution expressions used outside of affine index expressions and loop bounds are non-affine. This means not all the information is modeled/carried by our set of parameters, which is why e.g. the OpenMP code generation is required to scan the SCEVs of a ScopStmt for additional values that need to be transferred (See: getReferencesInSubtree).

Now, I think we should probably model some of these "hidden" scalar dependences more explicitly in the ScopStmt, but I agree that this probably nothing we should block this patch on. In fact, the immediate issue is already taken care of as the OpenMP code generation already scan's the SCEVs again.

It just took me one day to reason about this issue. I think the isSynthesisable part of this patch is fine.

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


I hoped to have explained the issue a little bit higher up. I don't think your patch can/will fix the issue I just explained, but we should probably sit down and reason about this in a separate patch. Within the model we use today, your patch is clearly an improvement and it does not regress any existing code.

I have two last requests/questions:

- As written earlier, I think we should make this just about the isSynthesisable()
- I wonder why we did not catch this earlier. We already check canSynthesize() in both buildPHIAccesses and in buildScalarDependences. Still, it seems we add unnecessary reads. Did you understand where these still sneaked through?

Best,
Tobias


http://reviews.llvm.org/D15687