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

[Jingyue Wu]

================
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) {
----------------
Chandler Carruth wrote:
> Just use nullptr as the initial value?
That would make foldPHINode return null for phi(undef, undef). 

================
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);
----------------
Chandler Carruth wrote:
> 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...)
Done. 

I agree. Will add a range for PHINode in a separate patch. 

================
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;
+    }
+  }
----------------
Chandler Carruth wrote:
> I think this would be slightly simpler as:
> 
>   if (isa<UndefValue>(IV))
>     continue;
> 
>   if (!CommanValue)
>     CommonValue = IV;
>   else if (CommonValue != IV)
>     return nullptr;
Done

================
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) {
----------------
Chandler Carruth wrote:
> I'd probably put this in a 'foldPHINodeORSelectInst' static helper so you can write the if below:
> 
>   if (Value *Result = foldPHINodeOrSelectInst(I)) {
done

================
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) {
----------------
Chandler Carruth wrote:
> How about just "Size"?
done.

================
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
----------------
Chandler Carruth wrote:
> 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?
done

http://reviews.llvm.org/D4659