[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:17
@@ -16,3 +16,1 @@
 // assumptions in addition to the original with conservative (default) aliasing
-// assumptions. The version of the loop making aggressive aliasing assumptions
-// will have all the memory accesses marked as no-alias. These two versions of
----------------
We can mark only memory operation for which RT checks are created.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:161
@@ -160,3 +173,2 @@
     AU.addRequired<ScalarEvolutionWrapperPass>();
-    AU.addRequired<TargetLibraryInfoWrapperPass>();
     AU.addPreserved<AAResultsWrapperPass>();
----------------
TargetLibraryInfo is not used in the code.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:172
@@ -171,3 +183,3 @@
         InvariantThreshold(LVInvarThreshold), LoadAndStoreCounter(0),
-        InvariantCounter(0), IsReadOnlyLoop(true) {
     initializeLoopVersioningLICMPass(*PassRegistry::getPassRegistry());
----------------
Removed unused fields or made them function scoped instead of class scoped. 

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:189
@@ -175,2 +188,3 @@
 
+private:
   AliasAnalysis *AA;         // Current AliasAnalysis information
----------------
Made class fields private because there is no need to have them public.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:305
@@ -304,3 +306,1 @@
-    DEBUG(dbgs() << "    Alias tracker type safety failed!\n");
-    return false;
   }
----------------
TypeCheck is needed when we with AliasSet with MustAlias. E.g. char * and char ** pointers may be aliased but they have different types.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:502
@@ -501,3 +802,1 @@
-          MDNode::concatenate(Inst.getMetadata(LLVMContext::MD_alias_scope),
-                              MDNode::get(Inst.getContext(), Scopes)));
     }
----------------
Loop Versioning provides API for annotating a loop with 'no alias' metadata. It's better not to duplicate this functionality.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:524
@@ -523,3 +819,1 @@
-  // Initial allocation
-  CurAST = new AliasSetTracker(*AA);
 
----------------
This is not safe. Replaced with unique_ptr.

================
Comment at: test/Transforms/LoopVersioningLICM/loopversioningLICM1.ll:15
@@ -14,3 +14,3 @@
 ; CHECK-NEXT: %arrayidx = getelementptr inbounds i32, i32* %var1, i64 %idxprom
-; CHECK-NEXT: store i32 %add, i32* %arrayidx, align 4, !alias.scope !2, !noalias !2
 ; CHECK-NEXT: %add8 = add nsw i32 %[[induction]], %add
----------------
Meaningless metadata. The instruction is not aliased with itself by default. Also it's better not to hard-coded ids as they can be changed.


https://reviews.llvm.org/D24934