[PATCH] D28962: Add BFI in constanthoisting pass and do the hoisting selectively

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:678
+      Instruction *Base =
+          new BitCastInst(ConstInfo.BaseConstant, Ty, "const", IP);
+      DEBUG(dbgs() << "Hoist constant (" << *ConstInfo.BaseConstant
----------------
davidxl wrote:
> There are three cases:
> 1) new insertion point introduced
> 2) the BB originally has a base constant use
> 3) the BB originally has a constant use but need rebase.
> 
> For case 1) and 3), new insertion is needed, but for 2) it can be skipped.
Correction: 2) can only be skipped when there are no other uses dominated by it -- so it is probably better to not to skip and rewrite the original use


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:684
+      // Emit materialization code for all rebased constants.
+      for (auto const &RCI : ConstInfo.RebasedConstants) {
+        for (auto const &U : RCI.Uses) {
----------------
davidxl wrote:
> The right way to fix is to move the rebasing code out of the IPSet traversal loop after all new base constant materialization insertion are done. Only those that got dropped in findConstantInsertionPoint need to be handled.
Discard the second part of the last comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D28962