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

[Mailing List "llvm-commits" via llvm-commits]

llvm-commits added a comment.

> Meinersbur added a comment.

> 

> In http://reviews.llvm.org/D13676#265724, @grosser wrote:

> 

> > the patch looks generally fine. I am only slightly concerned your removal of the instruction to multiple read/write accesses. As it seems to not be strictly necessary, I wonder if we should rather leave the multi read/write accesses in?

> 

> This was one of the things I was most happy to simplify. No more memory management of lists in lists, or the difficulty to understand when a MemoryAccess is connected to an instruction.


With this patch __but__ multiple accesses per instruction we would still
simplify both. All MemoryAccesses connected to an instruction would be
real (explicit) memory accesses (maybe) caused by that instruction.

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

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

> Atomic exchange is more interesting. Not sure how to model this, it could also be a single MemoryAccess of a special type. If not, it would still be at most one read and one write per instructions, for which we do not need the overhead of a list. (e.g. two maps InstructionToReadAccess+InstructionToWriteAccess; or a single map to a pair<MemoryAccess*,MemoryAccess*>)

> 

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

>  Comment at: include/polly/ScopInfo.h:749

>  @@ +748,3 @@

>  +  /// @brief Implicit loads at the beginning of a statement.

>  +  MemoryAccessVec LeadingReads;

>  +

> 

>  ----------------

> 

> jdoerfert wrote:

>  > Now that I think about it, maybe leading and trailing is not the best wording here. While it is literally true, it doesn't help to understand __why__ they are leading/trailing. Maybe if we combine leading with the DependentScalars from http://reviews.llvm.org/D13611 and and rename trailing writes in escaping writes the purpose/need becomes clear.

>  I don't know what DependentScalars are meant to be, but those reads are not 'dependent' nor necessarily scalars (but also PHIs).


1. DependentScalars are scalars that need to be recomputed in order to generate code for that statement (it says something like this in the comment). Leading loads are scalars that need to be reloaded in order to .. (copy text from above). Already the definition is so similar that we should merge those two.
2. Generally PHIs and PHI operands are considered scalars too.

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

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

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

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

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

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

>  @@ -3458,1 +3487,3 @@

>  +    return;

>  +  Acc->getStatement()->addLeadingLoad(Acc);

> 

>   }

> 

>  ----------------

> 

> grosser wrote:

>  > This pattern seems rather repetitive. It happens five times at least. Does it make sense to add the "if (!Acc) ..." part directly to addMemoryAccess?

>  No, Acc is added to different lists each time.

> 

> The alternative would be to have some detection logic in addMemoryAccess to which list the access it to add, but more error prone than simple NULL checks.


We could simply hand the list to the method instead of guessing?

In any case:
{

  ...
  if (!A)
    return
  B;

}

>
-

{

  ...
  if (A)
    B;

}

> http://reviews.llvm.org/D13676




- F945544: signature.asc <http://reviews.llvm.org/F945544>
- F945543: msg-20112-44.txt <http://reviews.llvm.org/F945543>


http://reviews.llvm.org/D13676