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


================
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.
D13616 also assumes there is exactly one explicit access per instruction (lookupExplicitAccessFor).

Function calls cannot be modeled with any fixed number of MemoryAccesses.

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

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

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.

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

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


http://reviews.llvm.org/D13676