[PATCH] D13676: [Polly] Store leading and trailing memory accesses separately

[Michael Kruse via llvm-commits]

Meinersbur added a comment.

In http://reviews.llvm.org/D13676#265946, @llvm-commits wrote:

> > ================
>
> >  Comment at: include/polly/ScopInfo.h:746
>
> >  @@ +745,3 @@
>
> >  +  /// @brief Mapping from instructions to explicit memory accesses.
>
> >  +  DenseMap<const Instruction *, MemoryAccess *> InstructionToAccess;
>
> >  +
>
> > 
>
> >  ----------------
>
> > 
>
> > grosser wrote:
>
> >  > I understand that with the currently supported features we now only have a single read/write access per statement, but I do not think this will/should hold in general. If we e.g. model function calls or intrinsics like _atomic_exchange_n this won't hold any more. Hence, I personally would prefer to not specialize too much here.
>
> >  http://reviews.llvm.org/D13616 also assumes there is exactly one explicit access per instruction (lookupExplicitAccessFor).
>
>
> Because at the moment there is. But that is different from cutting
>  support for something else. I only used the fact that there can be at
>  most one explicit access at the moment to simplify the algorithm.
>
> > Function calls cannot be modeled with any fixed number of MemoryAccesses.
>
>
> I am not sure how this generalization is helping the discussion or what
>  it even means. In any way, that sentence above doesn't suffice to
>  ignore the function call modeling problem.


How do you model a function calls that accesses data like this:

  for (int i = 0; i < n; i+=1)
     A[i] = xyz;

The number of accesses is only known at runtime, hence one cannot add a MemoryAccess for each write.

> > 'writes' are also not escaping. Escaping also already has a meaning of values that are used after the scop.

> 

> 

> Escaping the block/statement or escaping the SCoP, whatever. In the end

>  it is almost the same and it is not important. Important is that they

>  need to be communicated from the defining statement (where they escape)

>  to somewhere else and we do that via memory. That they are tailing the

>  statement is a mere side effect and isn't even like that at the moment.


This patch depends on http://reviews.llvm.org/D13487. Sorry, seems I didn't mark it as such.

> > For me, a name describing the content is sufficient. There can be multiple reasons why something is in there, which can be described in a comment.

> 

> 

> I don't understand this part. Are you saying leading/trailing are

>  descriptive enough to understand what is going on and why?


It is enough to know what the vectors contains, which is all that matters for their names. How elements are put in there and what they are used for is better explained in a comment.

> > ================

> 

> >  Comment at: lib/Analysis/ScopInfo.cpp:1375

> 

> >  @@ -1360,7 +1374,3 @@

> 

> > 

> 

> > - // Remove all memory accesses in @p InvMAs from this statement together

> 

> > - // with all scalar accesses that were caused by them. The tricky iteration

> 

> > - // order uses is needed because the MemAccs is a vector and the order in

> 

> > - // which the accesses of each memory access list (MAL) are stored in this

> 

> > - // vector is reversed. +  // Remove the MemoryAccess from all lists. for (MemoryAccess *MA : InvMAs) { ---------------- jdoerfert wrote: > You mentioned at some point we should extract the "removeMemoryAccesses" from this function and I now see the need. In http://reviews.llvm.org/D13611 this happened we could use some fancy lambda magic like this to get rid of the weird traversal order if we can still get the same "runtime". Shouldn't that be a separate patch?

> 

> 

> Yes, and if it would happen __prior__ to any of those we would have

>  smaller diffs, thus my comment.


I cannot work on hypothetical patches. When I asked that editing ScopStmt could be refactored out, you said when necessary. It hasn't become more necessary with this patch. However, I'd welcome if you commit such refactoring either prior of after this patch.

Could you answer withing the web interface? Phabricator screws up formatting from emails and has "llvm-commits" as author.


http://reviews.llvm.org/D13676