On 08/26/2016 07:03 AM, Bernd Schmidt wrote:
On 08/01/2016 03:42 AM, Segher Boessenkool wrote:
This is the second version. Concern was renamed to component, and all
other comments were addressed (I hope).
Not really, I'm afraid. There still seems to be no detailed explanation
of the algorithm. Instead, there is a vague outline (which should be
expanded to at least the level of detail found e.g. in tree-ssa-pre.c),
and inside the code there are still meaningless comments of the form
I think Segher and I can work towards this. The placement algorithm is
where I think the main focus will need to be. Segher has some text for
that, but we may need to supplement with a CFG or two to help clarify.
/* Deselect those epilogue components that should not be inserted
for this edge. */
which don't tell me anything about what the intention is (why should
they not be inserted?). The result is that as a reader, I still find
myself unable to determine whether the algorithm is correct or not.
Deselection is more about pruning components that we initially thought
were separate shrink wrap candidates, but upon deeper inspection we can
not handle. And it's all target driven.
Deselection has to run after placement computation because deselection
is a property of the desired insertion site (which is why this hook
passes in the desired edge where we want to insert code). Deselection
runs prior to actual generation & insertion of the sequences.
Deselection affects the ability to separately shrink wrap that component
globally.
So it does affect correctness, but it's pretty easy to see that its safe.
Worse, I'm still not entirely certain what it is intended to achieve: I
asked for a motivating example or two, but haven't seen one in the
comments anywhere. My most general question would be why the algorithm
for placing individual prologue components would be different from the
regular shrink-wrapping algorithm for full prologues. Examples and/or
arguments relating to how this new code acts with regard to loops are
also particularly needed.
I think Segher and Michael covered this in the various follow-ups. I'm
now comfortable with the overall goals.
Right now we have a single copy of the prologue/epilogue. That single
copy might get shrink-wrapped, but at the end of the day, it's still a
single copy that gets executed at most once.
If we relax the single copy and single execution traits, then we get
more freedom to place the prologues/epilogues and less frequently
executed points.
We get even more flexibility by handling components of the
prologue/epilogue separately.
Some comments in these areas will be appropriate, but again, I'm
comfortable with the overall direction this work has gone.
So, I think improvement is necessary in these points, but I also think
that there's room for experimental verification by way of self-tests. If
done thoroughly (testing the transformation on several sufficiently
random CFGs and maybe some manually constructed ones) it would go a long
way towards showing that at least we don't have to worry too much about
miscompilations. That's what I've been working on, and an initial patch
is below. This is incomplete and posted more as an RFD since we're
getting towards the end of the week: there are gaps in the test
coverage, and it currently fails the selftest. I assume the latter is a
problem with my code, but it wouldn't hurt if you could take a look;
maybe I misunderstood something entirely about what the separate
shrink-wrapping is supposed to achieve, or maybe I messed up the
algorithm with my changes.
I think the lack of test coverage is something we'll want to address.
jeff
