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

[Wei Mi via Phabricator via llvm-commits]

wmi 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,
----------------
davidxl wrote:
> assert !BBs.count(Entry) is true
Ok. 


================
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) {
----------------
davidxl wrote:
> 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?
Ok, will fix it.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:178
+      // Find another BB in BBs dominating current BB. Stop.
+      if (Node != BB && BBs.count(Node))
+        break;
----------------
davidxl wrote:
> 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));
>        
> 
That looks simpler. Will fix it.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:201
+  }
+
+  // Sort the nodes in WorkSet in top-down order and save the nodes
----------------
davidxl wrote:
> early return when MadeChange == false?
MadeChange == false here just means we didn't find a BB from BBs dominated by another one in BBs, but we may still do hoisting work in the following, like hoist a const materialization out of loop.


================
Comment at: lib/Transforms/Scalar/ConstantHoisting.cpp:234
+
+      // If MadeChange is false, we will return empty BBs.
+      BBs.clear();
----------------
davidxl wrote:
> Why return empty BBs?
BBs return the locations where to insert hoisted const materialization. If we decide not to hoist anything, i.e., MadeChange is false, we will return empty BBs saying nothing will be inserted.


Repository:
  rL LLVM

https://reviews.llvm.org/D28962