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

[Chandler Carruth]

Sorry. I tried to get back to this a couple of times but it required a lot of thought. And then your pings kept hitting me at bad times. =/

================
Comment at: lib/Transforms/Scalar/SROA.cpp:659-678
@@ -688,1 +658,22 @@
         S.DeadOperands.push_back(U);
+        // When the condition of the select is undef, we cannot simply replace U
+        // with undef. For instance, suppose loading from %U or %other does not
+        // trap. Then "load (select undef, %U, %other)" does not trap either.
+        // However, if we simply replace %U with undef, "load (select undef,
+        // undef, %other)" may trap because the select may return the first
+        // operand "undef".  Therefore, while we clobber U, we should have the
+        // select point to the other operand.
+        if (SelectInst *SI = dyn_cast<SelectInst>(&I)) {
+          if (isa<UndefValue>(SI->getCondition())) {
+            Type *BooleanType = SI->getCondition()->getType();
+            if (I.getOperandUse(1) == *U) {
+              // select undef, *U, other -> select 0, undef, other == other
+              I.setOperand(0, ConstantInt::getFalse(BooleanType));
+            } else {
+              assert(I.getOperandUse(2) == *U &&
+                     "at least one of the operand uses should be *U");
+              // select undef, other, *U -> select 1, other, undef == other
+              I.setOperand(0, ConstantInt::getTrue(BooleanType));
+            }
+          }
+        }
----------------
This is pretty horrible. It makes me question the entire approach of cleaning up instructions here.

I hate phase ordering.

If we're going to go down the rabbit hole here if using instsimplify within SROA to address phase ordering issues, I think it would be much cleaner to do as part of the recursive user walk for the pointer value, and to actually RAUW the values as they simplify rather than doing the dead-operand-tracking thing here. Consider -- if we don't RAUW then we won't notice if doing so would cause some user of the select (perhaps another select or PHI) to collapse to all inputs being the same.

But that requires parameterizing PtrUseVisitor, teaching it to use instsimplify, teaching it to track RAUWs correctly, etc etc etc. Yuck. Absolutely terrible. This is why phase ordering problems are hard.


Ultimately, I think the suggestion to use instsimplify is wrong here. We really only want to handle the simplifications that arise *because* of mem2reg, and the primary one there is when all inputs to the PHI node become the same because we promoted them all to the same register (rather than N different loads). We don't need or want to handle the more complex simplifications because other passes will.

If we want to make SROA really powerful against phase ordering issues like this, I think it would need serious architectural changes.


So can we go back to the simpler patch after my round of nit-picky comments? Because I suspect that one is essentially "ready to go".

http://reviews.llvm.org/D4659