[PATCH] [SROA] Fold a PHI node if all its incoming values are the same

[Chandler Carruth]

Yea, this looks nice. I have a bunch of nit-picky comments below... Nothing really significant.

================
Comment at: lib/Transforms/Scalar/SROA.cpp:332
@@ +331,3 @@
+static Value *foldPHINode(PHINode &PN) {
+  Value *CommonValue = UndefValue::get(PN.getType());
+  for (unsigned I = 0, E = PN.getNumIncomingValues(); I != E; ++I) {
----------------
Just use nullptr as the initial value?

================
Comment at: lib/Transforms/Scalar/SROA.cpp:333
@@ +332,3 @@
+  Value *CommonValue = UndefValue::get(PN.getType());
+  for (unsigned I = 0, E = PN.getNumIncomingValues(); I != E; ++I) {
+    Value *IV = PN.getIncomingValue(I);
----------------
I prefer to only use "I" (capitalized) for iterators. I'd use 'i' or 'Idx' here.

(and we should really add an iterator and range adaptor set to PHINode for these so you can use a range based for loop...)

================
Comment at: lib/Transforms/Scalar/SROA.cpp:336-344
@@ +335,11 @@
+    // Treat UndefValue as a wildcard.
+    if (isa<UndefValue>(IV) || isa<UndefValue>(CommonValue)) {
+      if (!isa<UndefValue>(IV))
+        CommonValue = IV;
+      continue;
+    }
+    if (CommonValue != IV) {
+      CommonValue = nullptr;
+      break;
+    }
+  }
----------------
I think this would be slightly simpler as:

  if (isa<UndefValue>(IV))
    continue;

  if (!CommanValue)
    CommonValue = IV;
  else if (CommonValue != IV)
    return nullptr;

================
Comment at: lib/Transforms/Scalar/SROA.cpp:676-681
@@ -662,11 +675,8 @@
 
-    // For PHI and select operands outside the alloca, we can't nuke the entire
-    // phi or select -- the other side might still be relevant, so we special
-    // case them here and use a separate structure to track the operands
-    // themselves which should be replaced with undef.
-    // FIXME: This should instead be escaped in the event we're instrumenting
-    // for address sanitization.
-    if (Offset.uge(AllocSize)) {
-      S.DeadOperands.push_back(U);
-      return;
+    Value *Result = nullptr;
+    if (PHINode *PN = dyn_cast<PHINode>(&I)) {
+      Result = foldPHINode(*PN);
+    } else {
+      Result = foldSelectInst(cast<SelectInst>(I));
     }
+    if (Result != nullptr) {
----------------
I'd probably put this in a 'foldPHINodeORSelectInst' static helper so you can write the if below:

  if (Value *Result = foldPHINodeOrSelectInst(I)) {

================
Comment at: lib/Transforms/Scalar/SROA.cpp:699
@@ -695,5 +698,3 @@
     // See if we already have computed info on this node.
-    uint64_t &SelectSize = PHIOrSelectSizes[&SI];
-    if (!SelectSize) {
-      // This is a new Select, check for an unsafe use of it.
-      if (Instruction *UnsafeI = hasUnsafePHIOrSelectUse(&SI, SelectSize))
+    uint64_t &PHIOrSelectSize = PHIOrSelectSizes[&I];
+    if (!PHIOrSelectSize) {
----------------
How about just "Size"?

================
Comment at: test/Transforms/SROA/phi-and-select.ll:567-568
@@ +566,3 @@
+}
+; CHECK-LABEL: @simplify_phi_nodes_that_equal_slice_2(
+; CHECK-NOT: alloca
----------------
I like to have all my CHECKs within the function body so I can find them and they don't get spliced. I also try to attach them to the instruction they reference. Can you put the label at the start of the function and then the -NOT after the alloca?

http://reviews.llvm.org/D4659