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

[Tobias Grosser via llvm-commits]

grosser added a subscriber: grosser.
grosser added a comment.

> Meinersbur added a comment.

> 

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

> 

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

> 

> 

> If this is the rationale, we should modify canSynthesize() to return false for values that are not in the SCoP's params.

> 

> Note that synthesizing is useful to remove scalar dependences:

> 

>   void func(i32 %x)

>   

>   Stmt1:

>     %1 = add i32 %x, 5

>   

>   Stmt2:

>     use(%1)

> 

> canSynthesize() returning true causes the add to be %1 to disappear, because it will be synthesized into Stmt2.


Right. That's one of the reasons it exists. So no, I do not think we 
want it to return false either, but probably want to find a way to keep 
track of these additional scalar references while still being able to 
look through SCEVs.

Anyhow, let's just keep this in mind.

> > - As written earlier, I think we should make this just about the isSynthesisable()

> 

> 

> OK, I will update this patch.

> 

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

> 

> 

> Because there is no check for canSynthesize() in buildPHIAccesses() when handling the case where the incoming value is not defined within the same Stmt.


Perfect. I think this info will be great in the commit message. In fact, 
it would probably be more educational to add the canSynthesize precisely 
at this spot. Then your next patch will show nicely that multiple 
canSynthesize() calls will be replaced by just one.

Best,
Tobias


http://reviews.llvm.org/D15687