[PATCH] [PlaceSafepoints] Use the analysis infrastructure rather than an inner pass manager

[Philip Reames]

On 05/13/2015 12:21 PM, Daniel Berlin wrote:
> On Wed, May 13, 2015 at 12:11 PM, Philip Reames
> <listmail at philipreames.com> wrote:
>> Hi sanjoy, igor-laevsky, AndyAyers, pgavlin, hfinkel, rkotler,
>>
>> I would really appreciate careful review on the analysis update logic in the newly added function.
>>
>> PlaceSafepoints was previously using an inner pass manager to run an analysis pass *after* modifying the IR to remove unreachable blocks.  In an effort to get rid of the inner pass manager (and the various dominator tree recalcs it implied), I switched to using the analysis infrastructure to actually get the various analysis needed and convert the inner pass manager stuff into a small non-pass helper class.
>>
>> However, I couldn't figure out how to keep the analysis results up to date over a call to removeUnreachableBlocks.  That routine freely modifies the CFG in non-trivial ways with no provisions for updating any of dominator tree
> Can you describe this in more detail?
> It simply seems to
> 1. Constant fold terminators
> 2. Remove dead blocks (including cycles)
>
> This doesn't seem all that difficult to update dominators for, and
> making it update would certainly be helpful to other passes.  In fact,
> #2 is trivial, #1 is the only slightly tricky case.
>
> If you can give a before and after example of what you think is a
> difficult update, i'd love to see it.
There's no fundamental issue that makes it impossible; it's just a lot 
of work for minimal gain.  From a practical perspective, you'd need to 
update each of the cases in markAliveBlocks, and everything in 
changeToUnreachable, changeToCall, ConstantFoldTerminator.  I could 
probably get each case, but the amount of code churn involved would be 
material and doesn't seem justified.  Not to mention the review burden 
and likely subtle breakage that would result.
> (In general, updating dominators for unreachability is pretty easy,
> because if any node strictly dominated by an unreachable blocks is
> also unreachable, so you can usually just drop the whole subtree, and
> pull everything else up by one)
This is more or less what the patch is doing.  The eraseNode interface 
expects to be given only leaf nodes, so I make everything a leaf, then 
delete it.  Do you have a strong preference for doing this differently?