On 08/14/2015 10:54 AM, Richard Biener wrote:
> Putting in m_fn looks backwards to me - it looks like we only need to remember
> the entry and exit BBs and the number of blocks. 

Fixed.

> In fact initializing
> dom_info from that would allow it to work on SESE regions as well?

Can't tell for sure. We would need a way to iterate through region's basic
blocks (I'm not familiar with that code, but it seems to me that they are not
contiguous: build_sese_loop_nests function iterates through the region by
iterating through all BBs and filtering out ones which are not in region). Also
we have a mapping from some global BB index to it's number in DFS-order walk,
i.e. we will either need to allocate enough space for it, or use a hash map
instead of an array. Never the less, initializing start/end blocks and "reverse"
flag in constructor makes the code look cleaner because we avoid repeating these
initializations.

> if you are here please fix the 'extern' vs. w/o 'extern' inconsistencies as well
> (we prefer 'extern').
Reverted (see later).

On 08/14/2015 09:20 PM, Jeff Law wrote:
> It looks like your patch is primarily concerned with converting all the internal
> stuff into a C++ style and not exposing a class to the users of dominance.h.
> Correct?
Well, sort of. But I think this class also provides some API (though it's used
internally in dominance.c): it computes some initial dominance tree and other
functions either query it or update incrementally. And, as Richard said, this
class could be modified to be used on SESE regions.

> I could argue that those kind of changes are independent of turning dom_info
> into a real class and if they're going to go forward, they would have to stand
> alone on their merits and go in independently if turning dom_info into a class
> (which AFIACT is the meat of this patch).
Actually I thought that putting all these non-functional changes into a single
patch would make the churn less (after all, it's a single commit). But I
understand this rather obvious cue, that these changes are, well, unlikely to be
accepted.
So, the updated patch contains only the dom_info-related changes.

> Umm, isn't ENABLE_CHECKING defined in auto-host.h (as set up by configure?)
> What's the reason for this change?
>
> Is the issue that auto-host.h won't define checking at all for --disable-checking?
Yes, it does not get defined even for --enable-checking=release.

>
> I think that the ENABLE_CHECKING conversion from #ifdef testing to testing for a
> value should probably be done separately.  It also probably has higher value
> than this refactoring.
Well, I'll try to work on that. After all, it's the most frequently used
condition for conditional compilation in GCC. And removing part of #ifdef-s will
probably help to avoid some errors (e.g. warnings which only appear in "release"
builds).

>
>
>> +
>> +  /* The function being processed.  */
>> +  function *m_fn;
> So presumably the idea here is to avoid explicitly hitting cfun which in theory
> we could recompute the dominance tree for another function. But is that really
> all that useful?
No, I was thinking more of moving toward having a self-contained compilation
context and being able to run compilation in multiple threads. I know that we
are far away from this goal but never the less. BTW, do we actually have such
goal or not? :)

>
> I'm a bit torn here.  Richi mentioned the idea of stuffing away a pointer to
> cfun looked backwards to him and he'd pretty stuffing away the entry, exit & #
> blocks and perhaps take us a step towards the ability to compute dominance on
> sub-graphs.
>
> The problem I see with Richi's idea now that I think about it more is keeping
> that information up-to-date.  Ie, if we've stuffed away those pointers, what
> happens if (for example) a block gets deleted from the graph.  What if that
> block happens to be the exit block we've recorded?
All this information is discarded after we have the dominator tree computed. The
tree itself is stored in et_node-s (which are "attached" to basic blocks).
dom_info is not used for incremental updates.


gcc/ChangeLog:

2015-08-15  Mikhail Maltsev <maltsevm@gmail.com>

        * dominance.c (new_zero_array): Define.
        (dom_info): Redefine as class with proper encapsulation.
        (dom_info::m_n_basic_blocks, m_reverse, m_start_block, m_end_block):
        Add new members.
        (dom_info::dom_info, ~dom_info): Define.  Use new/delete for memory
        allocations/deallocations.  Pass function as parameter (instead of
        using cfun).
        (dom_info::get_idom): Define accessor method.
        (dom_info::calc_dfs_tree_nonrec, calc_dfs_tree, compress, eval,
        link_roots, calc_idoms): Redefine as class members.  Do not use cfun.
        (calculate_dominance_info): Adjust to use dom_info class.
        (verify_dominators): Likewise.

-- 
Regards,
    Mikhail Maltsev