[PATCH] D22143: [SimplifyCFG] Rewrite SinkThenElseCodeToEnd

[James Molloy via llvm-commits]

jmolloy marked 3 inline comments as done.

================
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())) ||
----------------
sanjoy wrote:
> 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())`?
> also, why do you care about `I->mayHaveSideEffects() || I->mayReadOrWriteMemory())`?

Primarily to keep this as NFC as possible. It now handles loads and stores, but I hadn't done an audit of the possible things that could go wrong if we allowed movement of sideeffect instructions.

I suppose this is probably fine. Would you like me to delete the restriction?

================
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) {
----------------
sanjoy wrote:
> Can the number of operands vary between instructions in `Inst`?  How about if they are all readnone calls with different # of arguments?
This is enforced by Instruction::isSameOperationAs.


Repository:
  rL LLVM

https://reviews.llvm.org/D22143