[PATCH] D11355: Fix for PR24179 - mem2reg generates incorrect code

[Sanjoy Das]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

LGTM with comments addressed.


================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:429
@@ -429,2 +428,3 @@
+/// return false.  This is necessary in cases where, due to control flow, the
 /// alloca is potentially undefined on some control flow paths.  e.g. code like
 /// this is potentially correct:
----------------
I'd make this comment more explicit now:

```
/// alloca is undefined on only some control flow paths.  e.g. code like
/// this is correct in LLVM IR:
///
///  // A is an alloca with no stores so far
///  for (...) {
///    int t = *A;
///    if (!first_iteration)
///      use(t);
///    *A = 42;
///  }
///
```

================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:476
@@ +475,3 @@
+      if (I == StoresByIndex.begin())
+        // If there is no store before this load, bail out (load may be affected
+        // by the following stores - see main comment).
----------------
I'd put the `StoresByIndex.empty` special case here -- that way all of the logic dealing with "cannot find a previous store" lives at one place.

================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:568
@@ -559,2 +567,3 @@
     // linear sweep over the block to eliminate it.
     if (Info.OnlyUsedInOneBlock) {
+      if (promoteSingleBlockAlloca(AI, Info, LBI, AST)) {
----------------
I'd use `&&` here.


http://reviews.llvm.org/D11355