[PATCH] D22143: [SimplifyCFG] Rewrite SinkThenElseCodeToEnd

[David Majnemer via llvm-commits]

majnemer added a subscriber: majnemer.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1388
@@ +1387,3 @@
+  // common PHI instruction in the successor.
+  if (!llvm::all_of(Insts, [](const Instruction *I) {
+        // These instructions may change or break semantics if moved.
----------------
hans wrote:
> I know I'm oldfashioned, but I'd find this easier to read as a simple loop over Insts rather than all_of :-)
> 
> Since we want to return false for the cases below, we might as well do that directly rather than returning false in the lambda and then checking that return value.
I think the llvm:: is a little superfluous here.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1395
@@ +1394,3 @@
+        if (!isa<StoreInst>(I) && !isa<LoadInst>(I) &&
+            (I->mayHaveSideEffects() || I->mayReadOrWriteMemory()))
+          return false;
----------------
The comment says "change memory" but you are using `mayReadOrWriteMemory`.

Which is right?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1458
@@ +1457,3 @@
+    bool NeedPHI =
+        llvm::any_of(Insts, [&I0, O](const Instruction *I) {
+          return I->getOperand(O) != I0->getOperand(O);
----------------
I think llvm:: here is not needed.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1509
@@ +1508,3 @@
+  if (Blocks.size() != 2 ||
+      !std::all_of(Blocks.begin(), Blocks.end(), [](const BasicBlock *BB) {
+        auto *BI = dyn_cast<BranchInst>(BB->getTerminator());
----------------
Use llvm's all_of?


Repository:
  rL LLVM

https://reviews.llvm.org/D22143