[PATCH] D138641: [reg2mem] Add special handling to CatchSwitchInst

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM with some nits.



================
Comment at: llvm/lib/Transforms/Utils/DemoteRegToStack.cpp:100
+    if (CatchSwitchInst *CSI = dyn_cast<CatchSwitchInst>(InsertPt)) {
+      for (BasicBlock *Handler : successors(CSI)) {
+        new StoreInst(&I, Slot, &*Handler->getFirstInsertionPt());
----------------
nit: Omit braces for single-line for.


================
Comment at: llvm/lib/Transforms/Utils/DemoteRegToStack.cpp:153
+      break;
+  if (CatchSwitchInst *CSI = dyn_cast<CatchSwitchInst>(InsertPt)) {
+    // We need a separate load before each actual use of the PHI
----------------
This should be just `isa<>`, you are not using `CSI`.


================
Comment at: llvm/lib/Transforms/Utils/DemoteRegToStack.cpp:155
+    // We need a separate load before each actual use of the PHI
+    SmallVector<Instruction *, 4> users;
+    for (User *U : P->users()) {
----------------
nit: `users` -> `Users`.


================
Comment at: llvm/lib/Transforms/Utils/DemoteRegToStack.cpp:158
+    // We need a separate load before each actual use of the PHI
+    SmallVector<Instruction *, 4> users;
+    for (User *U : P->users()) {
----------------
Naville wrote:
> nikic wrote:
> > Why does this insert a load before uses, rather than doing the same as in the previous function (insert load in each successor)?
> Because otherwise we need another Map to record successor and the loadinst in it?
Okay, I get it now: If we insert in successor, we need to replace dominating uses. This is possible, but your way is much simpler.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138641/new/

https://reviews.llvm.org/D138641