[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:673
+    SmallPtrSet<Instruction *, 8> IPSet = findConstantInsertionPoint(ConstInfo);
+    assert(!IPSet.empty() && "IPSet is empty");
+
----------------
I am not sure why this is needed. If nothing is changed, it should be fine to skip the transformation for the constant (as well as rebase in uses) completely.  Without skipping, it basically force rebasing?


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:678
+      Instruction *Base =
+          new BitCastInst(ConstInfo.BaseConstant, Ty, "const", IP);
+      DEBUG(dbgs() << "Hoist constant (" << *ConstInfo.BaseConstant
----------------
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.


================
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) {
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D28962