[PATCH] D21496: LoadCombine Load Aliasing Fix

[Eli Friedman via llvm-commits]

eli.friedman added inline comments.

================
Comment at: /Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Scalar/LoadCombine.cpp:235
@@ +234,3 @@
+    DenseMap<const Value *, SmallVector<LoadPOPPair, 8>> &LoadMap) {
+  // If we combined or not.
+  bool Combined = false;
----------------
Useless comment.

================
Comment at: /Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Scalar/LoadCombine.cpp:248
@@ +247,3 @@
+
+    // Check the result.
+    if (RefInfo == ModRefInfo::MRI_Mod || RefInfo == ModRefInfo::MRI_ModRef) {
----------------
Useless comment.

================
Comment at: /Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Scalar/LoadCombine.cpp:249
@@ +248,3 @@
+    // Check the result.
+    if (RefInfo == ModRefInfo::MRI_Mod || RefInfo == ModRefInfo::MRI_ModRef) {
+      // Check the load count and try to aggregate.
----------------
Convention is to write this as "if (RefInfo & MRI_Mod)".

================
Comment at: /Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Scalar/LoadCombine.cpp:251
@@ +250,3 @@
+      // Check the load count and try to aggregate.
+      if (Loads->second.size() > 1 && aggregateLoads(Loads->second))
+        Combined = true;
----------------
Isn't this missing the step of sorting Loads->second?

================
Comment at: /Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Scalar/LoadCombine.cpp:261
@@ +260,3 @@
+      // Early exit for store.
+      if(isa<StoreInst>(V))
+        return Combined;
----------------
What's the justification for the early exit here? Testcase?

================
Comment at: /Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Scalar/LoadCombine.cpp:265
@@ +264,3 @@
+      // Update the end iterator and continue.
+      LoadMapEnd = LoadMap.end();
+      continue;
----------------
Is updating the end iterator actually necessary?

================
Comment at: /Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Scalar/LoadCombine.cpp:269
@@ +268,3 @@
+
+    // Update the iterator.
+    ++Loads;
----------------
Useless comment.

================
Comment at: /Users/rriddle/Desktop/llvm/llvm/test/Transforms/LoadCombine/load-combine-alias.ll:9
@@ +8,3 @@
+  %a = alloca [4 x i32], align 16
+  %b = alloca [4 x i32], align 16
+  %arrayidx = getelementptr inbounds [4 x i32], [4 x i32]* %b, i64 0, i64 0
----------------
Maybe it would be better to use noalias argument pointers instead of allocas?  This testcase is a little weird because you're loading undef.

================
Comment at: /Users/rriddle/Desktop/llvm/llvm/test/Transforms/LoadCombine/load-combine-alias.ll:33
@@ +32,3 @@
+entry:
+  %a.addr = alloca i32*, align 8
+  %b.addr = alloca i32*, align 8
----------------
The allocas seem unnecessary here.


http://reviews.llvm.org/D21496