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

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Michael,

the patch looks generally fine. My am only slightly concerned your removal of the instruction to multiple read/write accesses. As this does not seem to


================
Comment at: include/polly/ScopInfo.h:746
@@ +745,3 @@
+  /// @brief Mapping from instructions to explicit memory accesses.
+  DenseMap<const Instruction *, MemoryAccess *> InstructionToAccess;
+
----------------
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.

================
Comment at: include/polly/ScopInfo.h:885
@@ -879,6 +884,3 @@
 
-  /// @brief Return the (scalar) memory accesses for @p Inst if any.
-  MemoryAccessList *lookupAccessesFor(const Instruction *Inst) const {
-    auto It = InstructionToAccess.find(Inst);
-    return It == InstructionToAccess.end() ? nullptr : It->getSecond();
-  }
+  /// @brief Return the list of traling implicit stores.
+  const MemoryAccessVec &getTrailingWrites() const { return TrailingWrites; }
----------------
trailing

================
Comment at: lib/Analysis/ScopInfo.cpp:3488
@@ -3458,1 +3487,3 @@
+    return;
+  Acc->getStatement()->addLeadingLoad(Acc);
 }
----------------
This pattern seems rather repetitive. It happens five times at least. Does it make sense to add the "if (!Acc) ..." part directly to addMemoryAccess?


http://reviews.llvm.org/D13676