[PATCH] D22143: [SimplifyCFG] Rewrite SinkThenElseCodeToEnd

[James Molloy via llvm-commits]

jmolloy marked 10 inline comments as done.
jmolloy added a comment.

Thanks all for the comments! Patch updated.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1395
@@ +1394,3 @@
+        if (!isa<StoreInst>(I) && !isa<LoadInst>(I) &&
+            (I->mayHaveSideEffects() || I->mayReadOrWriteMemory()))
+          return false;
----------------
majnemer wrote:
> The comment says "change memory" but you are using `mayReadOrWriteMemory`.
> 
> Which is right?
mayReadOrWriteMemory was intended to be a proxy for "sideeffects". mayHaveSideEffects() is better.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1423
@@ +1422,3 @@
+          return areValuesTriviallySame(I->getOperand(O), I->getIterator(),
+                                        I0->getOperand(O), I0->getIterator());
+        }))
----------------
sanjoy wrote:
> Does this work with token types (which cannot be PHI'ed)?
The checks to isEHPad() catch all instructions that can take or return token types apart from catchret, which as a terminator can never be considered for sinking here.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1470
@@ +1469,3 @@
+                               Op->getName() + ".sink", &BBEnd->front());
+    for (auto *I : Insts)
+      PN->addIncoming(I->getOperand(O), I->getParent());
----------------
sanjoy wrote:
> You at least need to handle token types here.
Tokens should never ever get here. I've added an assert to that effect.


Repository:
  rL LLVM

https://reviews.llvm.org/D22143