...
We've already touched on the need to limit the backwards walk out of
this code. Limiting based on the product of # phi args * number of phis
visited, recursion depth, or number of SSA_NAME_DEF_STMTs visited all
seem reasonable to me.
I do think Richi's suggestion for figuring out a suitable inflection
point is reasonable.
It's easy enough to add here. But I know I've introduced other
algorithms that recurse on SSA_NAME_DEF_STMT, and I'm quite sure
others predate those. To make a difference I think we need to
review at least the one most likely to be exposed to this problem
and introduce the same limit there. I could probably fix the ones
I wrote reasonably quickly, but making the change to the others
would be much more of a project. I looked to see how pervasive
this might be and here is just a small sampling of things that
jumped out at me in a quick grep search:
* compute_builtin_object_size (for _FORTIFY_SOURCE)
* compute_objsize (for -Wstringop-overflow)
* get_range_strlen
* maybe_fold_{and,or}_comparisons in gimple-fold.c
* -Warray-bounds (computing an offset into an object)
* -Wrestrict (computing an offset into an object)
* -Wreturn-local-addr (is_addr_local)
* -Wuninitialized (collect_phi_def_edges)
I don't see the recursion in maybe_fold_{and,or}_comparisons.
I didn't study any of these very carefully, I just looked for uses
of SSA_NAME_DEF_STMT around recursive calls. same_bool_comparison_p
looks like it calls itself with that result. I could be wrong.
The others all smell like they might be yours ;) (besides object-size
maybe but that one is limited by having a cache - hopefully also
used when not used in the compute-everything mode).
It doesn't matter who introduced them. What I'm saying is that
if the recursion is a problem it should be limited everywhere,
not just in one place. A change with global scope like that would
best be made independently of other unrelated changes, to minimize
the risk of introducing bugs and then the difficulty of debugging
and fixing them.
Given how wide-spread this technique seems to be, if the recursion
is in fact a problem it's just as likely (if not more) to come up
in the folder or in BOS or some other place as it is here. So if
it needs fixing it seems it should be done as its own project and
everywhere (or as close as we can get), and not as part of this
integration.
It's never an excuse to add new cases though and adding a limit
is _really_ simple.
An "excuse?" I'm just explaining that I would prefer to introduce
the limit independently of the integration.
Sure, it is simple in that one place. I said that above. It's
also probably relatively simple in each of the other instances
(at least those I'm familiar with). It's just cleaner and safer
to add it independently of other changes, that's all.
Anyway, I posted a simple patch to introduce the limit. I'm also
changing the integrated pass to make use of it once it's committed.
If it's considered necessary I'm also willing to adjust the rest
of the algorithms I introduced to respect the limit.
Thanks
Martin
