[PATCH] D106317: [SimplifyCFG] performBranchToCommonDestFolding(): form sudo-LCSSA before cloning instructions (PR51125)

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1108
+      if (auto *PN = dyn_cast<PHINode>(UI)) {
         SSAUpdate.RewriteUse(U);
+      } else {
----------------
Do we still need SSAUpdater here? Can this be just `if (PN->getIncomingBlock(U) != BB) U.set(NewBonusInst);` or so?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:3080
+  // and that makes miscompiles easy. Let's make it explicit instead,
+  // by forming sudo-LCSSA.
+  for (Instruction &BonusInst :
----------------
This comment would really benefit from a description of what problem this is solving and how it does so. "makes miscompiles easy" doesn't really tell us anything and "sudo-LCSSA" form is somewhat confusing when not talking about loops.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:3088
+      return UI->getParent() != BB || !BonusInst.comesBefore(UI);
+    };
+
----------------
What is an "offending" use? Based on the implementation, it's a use that is potentially not dominated by BonusInst.

I'd personally invert the predicate and call it `IsTriviallyDominatedUse` or something.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:3098
+
+    // Into each successor block of BB, insert a PHI node, that recieves
+    // the BonusInst when coming from it's basic block, or poison otherwise.
----------------
receives


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106317