On 07/16/2015 05:03 AM, Martin Liška wrote:
So a general question. We're passing in STRICT to several routines, which is
fine. But then we're also checking M_TAIL_MERGE_MODE. What's the difference
between the two? Can they be unified?
Hello.
I would say that STRICT is a bit generic mechanism that was introduced some
time before. It's e.g. used for checking of THIS arguments for methods and make
checking
more sensitive in situations that are somehow special.
The newly added state is orthogonal to the previous one.
Fair enough. There's some cases where we've documented STRICT, and
others where we haven't.
If STRICT flag is true, version must match strictly
Appears as documentation for STRICT. It seems like it'd be better to
describe what "strictly" means here.
Elsewhere we have comments like:
Be strict in case of tail-merge optimization
Which tends to confuse things a bit. Perhaps something more like:
In the case of tail merge optimization, XYZ must match
It seems like a nit, but to someone else reading the code I don't think
the distinctions are all that clear right now, so if we can improve
things, it'd be good.
I didn't find this comment particularly useful in understanding what this
function does. AFAICT the function looks as the uses of the LHS of STMT and
verifies they're all in the same block as STMT, right?
It also verifies that the none of the operands within STMT are part of
SSA_NAMES_SET.
What role do those properties play in the meaning of "local definition"?
I tried to explain it more deeply what's the purpose of this function.
Thanks. It's much clearer now. We've actually got similar code in a
couple places (ifcvt). I wonder if we could unify those implementations
as a follow-up cleanup.
@@ -1037,4 +1205,60 @@ func_checker::compare_gimple_asm (const gasm *g1, const
gasm *g2)
return true;
}
+void
+ssa_names_set::build (basic_block bb)
Needs a function comment. What are the "important" names we're collecting here?
Is a single forward and backward pass really sufficient to find all the
important names?
In the backward pass, do you have to consider things like ASMs? I guess it's
difficult to understand what you need to look at because it's not entirely
clear the set of SSA_NAMEs you're building.
These questions and lack of function comment don't seem to have been
addressed yet.
> > +
> > +using namespace ipa_icf;
> > +using namespace ipa_icf_gimple;
Is this wise? Does it significantly help with conciseness within the tail
merging pass where it wants things ipa-icf and ipa-icf-gimple?
I'm not objecting at this point, I want to hear your thoughts.
I must agree with you, as I've started using both namespaces in tree-tail merge
pass,
it makes not much sense anymore. I suggest to come up with a namespace that will
encapsulate 'identical code folding'-related stuff. What about:
namespace icf
Sure if it helps and is clean. GCC does not have a policy against
"using namespace", but other codebases do (google for example) as it
does introduce some long term maintenance issues.
So when I see a "using namespace" of that nature, I'm naturally going to
question if it really helps in a significant way. If it does, then I
won't object. If it's not helping in a significant way, then I'm likely
to object.
I think the updated version is fine WRT namespaces.
?
/* Describes a group of bbs with the same successors. The successor bbs are
cached in succs, and the successor edge flags are cached in succ_flags.
@@ -1220,17 +1231,48 @@ gsi_advance_bw_nondebug_nonlocal (gimple_stmt_iterator
*gsi, tree *vuse,
}
}
+static bool
+check_edges_correspondence (basic_block bb1, basic_block bb2)
Needs a function comment.
Still needs function comment.
I think we're pretty close here. Most of the issues are comments -- I
still haven't looked *real* close at ssa_names_set::build. With a
function comment I ought to be able to look at it more closely in the
next (and hopefully final) iteration.
Jeff
