Re: [PATCH 1/2] C++-ify dominance.c

[Jeff Law <law at redhat dot com>]

On 08/14/2015 10:02 PM, Mikhail Maltsev wrote:
> On 08/14/2015 10:54 AM, Richard Biener wrote:
> > 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.
I suspect that lots of code would need to be tweaked to start handling 
regions.  As you note, I don't think  most of the walkers are 
region-aware, they just start at a given block and walk 
forward/backwards without any regard to whether or not they've left any 
predefined region.

> 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.
Just to be clear, I'm not objecting to what you've done, just noting 
that it doesn't have as much utility as class-ifying other things.


> > 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.
I wouldn't say it's a cue that whey wouldn't be accepted, but they're in 
the "danger zone".  Particularly since there was a conscious decision 
not to just change things to be C++-ish if there wasn't some kind of 
benefit.

It's not always clear what kind of cleanups that don't affect 
functionality are desirable.  There's certainly been cleanups through 
the years that I looked at and said to myself "why bother" at the time, 
only to look back years later and say "in retrospect that was a good idea".

And there's others that I look at and say to myself, why don't we just 
use commit-hooks to ensure certain things never get into the source tree 
again (I'm thinking whitespace and formatting stuff that can be handled 
by gnu-indent in particular).

It's also not clear where the line is between cleaning up obvious junk 
while working in a hunk of code and separating those cleanups as their 
own independent change.  I personally tend to want to see these as their 
own changes as it more easily allows me to focus on the meat of a 
change.  Others almost certainly draw the line in a different location.

So to summarize, I'm not trying to discourage this kind of work, but 
make you aware of some of the issues/concerns for that class of 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.
Just wanted to make sure.  As you're aware, we're trying to move away 
from the conditional compilation and your change does take us a step in 
that direction, but I suspect tackling ENABLE_CHECKING is probably best 
done as an independent change so that we don't have these fragments all 
over the place to turn a defined/not-defined into something we can check 
with if ().


> > 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).
That would be greatly appreciated.

> > > +  /* 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? :)
We do have such a goal via the JIT project and more generally 
compile-servers.  I sometimes hesitate to mention the latter as I've 
seen so many compile server projects fail miserably after folks have 
invested enormous amount of time/energy.


> > 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.
Ah, in that case it shouldn't be a problem stuffing away those pointers 
instead of the cfun.

> 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.
OK for the trunk.

Thanks,
Jeff