[PATCH] D15706: [Polly] Follow uses to create value MemoryAccesses

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Michael,

this patch looks like a nice improvement. Also, I like that the patch is very targeted only changing the order a couple of memory writes. I only have some minor questions:

From the commit message:

> Because it does not catch all uses (e.g. those by PHIs), some code adds more scalar reads write on the fly, with relatively conditions and possible creation of MemoryAccesses that are not required.


Is this still correct? As far as I see, this patch does not reduce the number of memory accesses modeled. Maybe this functionality was already committed with the previous patch?

Also, I do not yet fully understand why we add new accesses in "test/ScopInfo/invariant-loads-leave-read-only-statements.ll". Was this what the sentence above was about? Could you maybe point out again the reason why we need to add more load accesses.
My feeling was that this is an [NFC] patch when ignoring the order of the generated memory accesses.


================
Comment at: lib/Analysis/ScopInfo.cpp:3872
@@ +3871,3 @@
+  // There cannot be an "access" for constants.
+  if (isa<Constant>(Value) || isa<BasicBlock>(Value))
+    return;
----------------
What about GlobalVariable constants. I assume we want to keep accesses to them, no?


http://reviews.llvm.org/D15706