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

[Johannes Doerfert via llvm-commits]

jdoerfert marked an inline comment as done.
jdoerfert added a comment.

In http://reviews.llvm.org/D13194#254243, @Meinersbur wrote:

> What's the motivational use case that cannot be handled by LICM?


I forgot to split the test cases, sorry. Here is one:

- http://reviews.llvm.org/D13195#2eebfa1a

also note the commit message (it is stated there).

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


================
Comment at: include/polly/CodeGen/IslNodeBuilder.h:209
@@ +208,3 @@
+  /// Note that if the runtime context implies the condition C conditional
+  /// execution is not necessary.
+  Value *preloadInvariantLoad(const MemoryAccess &MA,
----------------
Meinersbur wrote:
> Grammar? I don't understand this sentence
I'll simplify the sentence and add a comma.

================
Comment at: lib/Analysis/ScopInfo.cpp:1299
@@ +1298,3 @@
+    MemAccs.erase(MemAccsIt);
+  }
+
----------------
Meinersbur wrote:
> list::remove_if
I am unsure. What should I replace by list::remove_if and why?

================
Comment at: lib/Analysis/ScopInfo.cpp:2285
@@ +2284,3 @@
+
+    StmtIt = Stmts.erase(StmtIt);
+  }
----------------
Meinersbur wrote:
> Could you refactor removing a ScopStmt from a Scop into another function so it might be used by other code as well?
We can do that easily as soon as another usecase presents itself. If ppl insist we can do it now but I don't see the benefit at the moment.

================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:830
@@ +829,3 @@
+
+Value *IslNodeBuilder::preloadInvariantLoad(const MemoryAccess &MA,
+                                            isl_ast_build *Build,
----------------
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.


http://reviews.llvm.org/D13194