[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:164
+  // Nodes on the current path to the root.
+  SmallPtrSet<BasicBlock *, 8> Path;
+  // WorkSet will include every BB which dominates a BB in BBs,
----------------
assert !BBs.count(Entry) is true


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:170
+  // BB_1 but not including BB_2) will not be added into WorkSet.
+  SmallPtrSet<BasicBlock *, 16> WorkSet;
+  for (auto BB : BBs) {
----------------
It is simpler to document like this:

WorkSet includes any block 'BB' that is not strictly dominated by any other blocks in set 'BBs', and all nodes in the path in the dominator tree from Entry to 'BB'.

Also name WorkSet as Candidates?


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:178
+      // Find another BB in BBs dominating current BB. Stop.
+      if (Node != BB && BBs.count(Node))
+        break;
----------------
In the first iteration, this is always false, so perhaps change the loop into a do-while:

bool isCandidate = false;
do {
   Path.insert(Node);
    if (Node == Entry) {
         isCandidate = true;
         break;
    }
    Node = getIDom(Node);
} while (!BBs.count(Node));
       



================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:201
+  }
+
+  // Sort the nodes in WorkSet in top-down order and save the nodes
----------------
early return when MadeChange == false?


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:231
+      // If Node is not in BBs, we have done some hoisting.
+      if (InsertPtsFreq >= BFI.getBlockFreq(Node) && !NodeInBBs)
+        MadeChange = true;
----------------
NodesInBBs can not be true for Entry node.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:234
+
+      // If MadeChange is false, we will return empty BBs.
+      BBs.clear();
----------------
Why return empty BBs?


Repository:
  rL LLVM

https://reviews.llvm.org/D28962