[PATCH] D24934: [LICM] Add support of a new optimization case to Loop Versioning for LICM + code clean up

[Evgeny Astigeevich via llvm-commits]

eastig added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:403
@@ +402,3 @@
+/// \return true if a loaded pointer can have bounds.
+bool LoopVersioningLICM::canLoadedPtrHaveBounds(const LoadInst * Load) {
+  assert(Load);
----------------
ashutosh.nema wrote:
> This function looks for very specific pattern, need to generalize.
Yes, it's currently for very a specific pattern which we've got from benchmarks and real code.
To generalize the function I need to have more patterns. 

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:431
@@ +430,3 @@
+
+  if (!Load->hasOneUse() && !areOtherUsersOnlyMemOps(Load, GEP)) {
+    DEBUG(dbgs() << "     Failed: Non-memory operation different from GEP uses Load\n");
----------------
ashutosh.nema wrote:
> Why restricting to single load & its users as memory operations & GEP ?
Because this is the current pattern we want to recognized. More uses might create sophisticated DFG which is not worth to analyze. Such cases should be discovered first then get analyzed. 

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:446
@@ +445,3 @@
+
+  if (!GEP->hasOneUse()) {
+    DEBUG(dbgs() << "     Failed: GEP has more than one users.\n");
----------------
ashutosh.nema wrote:
> Why one use ?
The same answer as above. Lack of real use cases.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:489
@@ +488,3 @@
+  // should be used for making decision.
+  if (CurLoop->getParentLoop()) {
+    DEBUG(dbgs() << "    Loop nests are not supported.\n");
----------------
ashutosh.nema wrote:
> Current LoopVersioningLICM supports loop nest, i.e. it targets innermost loop.
> This behavior should be keep supported.
> 
The old cases work as before, nothing is changed.
The behaviour is only changed for the new cases. Loads are very heavy operations it is too dangerous to move them from an inner loop to an upper loop because the upper loop might have 1000 iterations and the inner loop might have 10. If there are aliased pointers a operation loading a pointer will be executed 11000 times instead of 10000 times: 1000 times in the RT checking basic block + 10000 times in the original loop. To make a proper decision an execution profile should be used.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:496
@@ +495,3 @@
+    const LoadInst * Load = dyn_cast<LoadInst>(Ptr);
+    if (!Load) {
+      DEBUG(dbgs() << "    It is not a loaded a pointer: " << *Ptr << "\n");
----------------
ashutosh.nema wrote:
> Earlier collected write to memory as well, why don't want to consider them here ?
LoopAccessAnalysis collects pointers in terms of Values. Writes are users of those pointers. So for each pointer in a loop we have an instruction defining it.


https://reviews.llvm.org/D24934