[PATCH] D31032: [LoadCombine] Avoid analysing dead basic blocks

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: lib/IR/LegacyPassManager.cpp:1283
 
+  // Scan the functions basic blocks in Reverse Post Order. This excludes dead
+  // basic blocks (it has been seen that the analysis in some passes could hang
----------------
bjope wrote:
> dberlin wrote:
> > 1. You need to define dead.
> > If you mean "forward unreachable", please use that.
> > 
> > 2. RPOT is not the optimal order for some problems (in fact, for some, it's optimally bad), and forcing that order seems ... concerning.
> > I'm pretty strongly against forcing a bb order in a generic pass infrastructure.
> > 
> > 3. It looks like we have 3 or 4 basic block passes.
> > 
> > Are they getting anything from being basicblockpasses?
> > 
> > 
> > It looks like they could all be trivially changed to functionpasses, and those that can't handle dead code, could just be changed to do what they need to.
> > 
> > For example, DCE has a dominator tree around, it can tell whether things are forward unreachable without wasting time doing a reverse postorder traversal.
> > 
> > IMHO, that is a better solution (and helps new PM) than forcing a pass order here.
> What do you mean by "forcing an order"?
> 
> We have not changed the order in which basic block passes are executed here. All we did was adding the IsAlive flag to tell the basic block passes if the BB it should operate on is "forward reachable" (from the function entry) or not. And then it is up to each BasicBlockPass to make a choice if it should operate on the BB or not.
> 
> The idea was that passes like the BasicBlockPassPrinter should be able to also print the dead blocks, whereas LoadCombine could avoid spending time on optimizing the code that most likely will be removed by DCE later anyway. So that is why we provide the flag instead of simply skipping to iterate over the "dead" blocks.
> 
Yes, you are right, i misread that part.
Note that RPOT is expensive and unnecessary here.
You could just use depth_first_search. Just use the version with an external visited set, and it will fill in the set for you.

I'm still strongly of the opinion to simply kill basicblockpass rather than add another O(basic blocks) walk that not everything needs.

Every other pass also properly handles dead or unreachable code themselves, without help from the pass infrastructure.




https://reviews.llvm.org/D31032