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

[Daniel Berlin]

On Wed, May 13, 2015 at 6:11 PM, Philip Reames
<listmail at philipreames.com> wrote:
>
>
> 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.

Fair enough.

>>
>> (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?

Not really, it's more "I hate it when we stop using code because it
doesn't update the things we want it to".  That's usually a sign of
"code we should be fixing or removing" (the former if it's good and
should be used, the latter if it's likely others will want to use it
but it doesn't really keep things up to date)

>