[PATCH] D13194: [Polly] Identify and hoist definitively invariant loads

[Michael Kruse via llvm-commits]

Meinersbur added a comment.

In http://reviews.llvm.org/D13194#254248, @jdoerfert wrote:

> In http://reviews.llvm.org/D13194#254243, @Meinersbur wrote:
>
> > Could you create a separate pass that modifies the ScopInfo, i.e. lib/Transform/HoistInvariantLoads.cpp
>
>
> I am unsure (not if I could but if it makes sense). This patch performs "licm" only as an optimization, true, but from http://reviews.llvm.org/D13195 onwards we will need it as a crucial part of the SCoP generation. Hence, I think it belongs (at least in the long run) in the ScoP class.


OK


================
Comment at: lib/Analysis/ScopInfo.cpp:1299
@@ +1298,3 @@
+    MemAccs.erase(MemAccsIt);
+  }
+
----------------
jdoerfert wrote:
> Meinersbur wrote:
> > list::remove_if
> I am unsure. What should I replace by list::remove_if and why?
Does this just remove all elements of MAL from MemAccs? Maybe if not by remove_if (std::std::set_difference), this could be probably rewritten using STL functions. It also looks fragile because it depends on the order of elements in MAL and MemAccs.

Please add at least a comment about what the code is supposed to do.

================
Comment at: lib/Analysis/ScopInfo.cpp:2238
@@ -2202,1 +2237,3 @@
+  collectAndCheckInvariantLoads(SD);
+  simplifySCoP();
 }
----------------
These do not initialize the Scop, but transform it.

================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:830
@@ +829,3 @@
+
+Value *IslNodeBuilder::preloadInvariantLoad(const MemoryAccess &MA,
+                                            isl_ast_build *Build,
----------------
jdoerfert wrote:
> Meinersbur wrote:
> > Can you write a comment about how the CFG will look like?
> I could, but it would just be the CFG from [[ http://reviews.llvm.org/D13194#c8db0cac | here ]] a couple of times in sequence.
Linking into files in Phabricator reviews doesn't work for me. It just jumps to a location before it lazily loads the diffs.

You could just refer to that code location in the comment.


http://reviews.llvm.org/D13194