Re: [PATCH v2 0/9] Separate shrink-wrapping

[David Malcolm <dmalcolm at redhat dot com>]

On Fri, 2016-08-26 at 15:03 +0200, 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
> 
> /* 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.
> 
> 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.
> 
> 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.
> 

Bernd: I'm very happy to see someone else using the selftest framework.

(My mailer isn't letting me quote the patch body, sorry).

I'm nervous about the build_random_cfg function: randomness in
selftests seems like a double-edged sword.  On the one hand, we can use
it to fuzz-test an optimization to rapidly gain a lot of coverage.  On
the other hand, does every host generate the same sequence of values?
Presumably we want everyone to be effectively running the same
selftests.

Is this using libiberty's implementation of "random", or can it use the
underlying host libc implementation?  Are there any cross-platform
guarantees on the sequence of values that are returned for a particular
seed?  I don't want us to have host-specific failures that turn out to
be due to host differences in the RNG.

Do we need to re-seed the RNG before each test?  (or to rework
libiberty's random to bundle up the RNG state in a class, and use
that).  Otherwise, the meaning of the test could change if someone adds
another random-using selftest before this one.

Maybe there's a need for some kind of selftest::rng class here?

On a unrelated note, should the various vfunc implementations be marked
with "FINAL OVERRIDE" (from coretypes.h), so that the compiler has a
better chance of devirtualizing them in a release build?

Hope this is constructive
Dave