[PATCH] D22143: [SimplifyCFG] Rewrite SinkThenElseCodeToEnd

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1263
@@ +1262,3 @@
+static bool areValuesTriviallySame(Value *V0, Value *V1) {
+  if (V0 == V1)
+    return true;
----------------
Why not use `Instruction::isIdenticalTo` or `Instruction::isIdenticalToWhenDefined`?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1295
@@ -1297,4 +1294,3 @@
   }
-  if (!FirstNonPhiInBBEnd)
-    return false;
 
+  // Prune out obviously bad instructions to move. Any non-store instruction
----------------
This is minor and optional to fix, but I'd write all of the `any_of` checks you have as `all_of` checks, I think that reads more natural -- "I want all of Insts to have this property, if not leave".

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1299
@@ +1298,3 @@
+  // common PHI instruction in the successor.
+  if (std::any_of(Insts.begin(), Insts.end(), [](const Instruction *I) {
+        return isa<PHINode>(I) || I->isEHPad() || isa<AllocaInst>(I) ||
----------------
Why not use `llvm::any_of`?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1301
@@ +1300,3 @@
+        return isa<PHINode>(I) || I->isEHPad() || isa<AllocaInst>(I) ||
+               (!isa<StoreInst>(I) && !isa<LoadInst>(I) &&
+                (I->mayHaveSideEffects() || I->mayReadOrWriteMemory())) ||
----------------
Instead of having a dense block of code here, I'd piecemeal put the comments down here, like:

```
if (is not load or store)
  return may have side effects or mayreadorwritememory; // why?
...
```

also, why do you care about `I->mayHaveSideEffects() || I->mayReadOrWriteMemory())`?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1322
@@ +1321,3 @@
+  NumPHIsRequired = 0;
+  for (unsigned O = 0, E = I0->getNumOperands(); O != E; ++O) {
+    if (std::any_of(Insts.begin(), Insts.end(), [&I0, O](const Instruction *I) {
----------------
Can the number of operands vary between instructions in `Inst`?  How about if they are all readnone calls with different # of arguments?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1324
@@ +1323,3 @@
+    if (std::any_of(Insts.begin(), Insts.end(), [&I0, O](const Instruction *I) {
+          return !areValuesTriviallySame(I->getOperand(O), I0->getOperand(O));
+        }))
----------------
Does `areValuesTriviallySame` take into account things like:

```
%a = load %x
*%x = 42;
%b = load %x
```

above `%a` and `%b` are trivially same, but won't compute the same value.


Repository:
  rL LLVM

http://reviews.llvm.org/D22143