Re: [PATCH] Fix PRs59125 and 54570

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Mon, Nov 18, 2013 at 5:25 AM, Richard Biener <rguenther@suse.de> wrote:
> On Fri, 15 Nov 2013, Richard Biener wrote:
>
>> On Fri, 15 Nov 2013, Jakub Jelinek wrote:
>>
>> > On Fri, Nov 15, 2013 at 02:56:51PM +0100, Richard Biener wrote:
>> > > Now that there is (finally :() a wrong-code testcase for the
>> > > PR54570 issue we can no longer ignore it (bah).  So the following
>> > > tries to paper over the fact that object-size sucks and disables
>> > > value-numbering of equal addresses the same before that pass
>> > > had a chance to finally look at the structure of the addresses.
>> > >
>> > > To make this "fix" suck less I moved the object-size pass before
>> > > the final FRE pass runs which is after IPA inlining and the
>> > > propagation of constants and addresses.  You won't catch
>> > > any "improvements" you'd get by memory CSE opportunities that
>> > > IPA inlining exposes, but you cannot have everything here.
>> >
>> > If it doesn't regress anything in the testsuite, I guess that is ok.
>> >
>> > > (IMHO object-size would should run during early optimizations)
>> >
>> > It can't, because inlining and some limited cleanup afterwards is essential
>> > for it.  Otherwise you'd regress not just for __builtin_object_size (x, 1),
>> > which admittedly is problematic since the introduction of MEM_REFs and
>> > various other changes, but also for __builtin_object_size (x, 0), which
>> > would be much more important.
>> >
>> > As discussed earlier, perhaps instead of checking cfun->after_inlining
>> > you could just introduce a new flag whether cfun contains any
>> > __builtin_object_size (x, {1,3}) calls, initialized by the gimplifier,
>> > propagated by the inliner and finally cleared again by objsz pass.
>>
>> But you'd need to pessimistically initialize it because if you inline
>> into a function with __builtin_object_size you may not previously
>> optimize.  You can of course analyze the cgraph to clear it for
>> functions that cannot end up being inlined into such function.
>> So that's effectively the same as ->after_inlining
>> minus losing the optimization that didn't end up with
>> __builtin_object_size after that.
>>
>> Not sure if it's worth all the trouble.  Arriving at a better
>> design for computing __builtin_object_size would be better ;)
>> (not that I have one)
>>
>> > Of course, if moving objsz earlier seems to work, it could stay where you
>> > put it, but the flag could make it clearer why you want to avoid certain
>> > optimizations.
>>
>> Well, all object-size testcases are pretty simplistic right now and
>> don't trigger the IPA inliner for example.
>>
>> > > Bootstrap / regtest pending on x86_64-unknown-linux-gnu.
>> > >
>> > > Similar candidate for the 4.8 branch.
>> >
>> > Please wait sufficiently long for trunk issues before you backport.
>>
>> Of course.
>
> So I had to do some more changes because doing objsz earlier (and
> separating it from strlenopt) exposes that the GENERIC folders (ugh)
> for strcat mess up points-to info as it folds strcat to
> memcpy (SAVE_EXPR <dest + strlen ()>, ...) which adds a new
> SSA name for the destination with no alias info associated.
>
> Rather than moving the non-constant builtin foldings to GIMPLE
> I chose to make sure to forward the constants in objsz and fold
> the stmts there (as PTA is re-run shortly after it).
>
> This then breaks some of the scan-tree-dumps in the strlenopt
> testcases because nobody has previously folded the _chk builtins
> with -1U size to non-_chk variants, so I have to adjust them
> (I didn't want to move the strlenopt pass as well).
>
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
>
> Any objections?
>
> Thanks,
> Richard.
>
> 2013-11-18  Richard Biener  <rguenther@suse.de>
>
>         PR tree-optimization/59125
>         PR tree-optimization/54570
>         * tree-ssa-sccvn.c (copy_reference_ops_from_ref): When inlining
>         is not complete do not treat component-references with offset zero
>         but different fields as equal.
>         * tree-object-size.c: Include tree-phinodes.h and ssa-iterators.h.
>         (compute_object_sizes): Apply TLC.  Propagate the constant
>         results into all uses and fold their stmts.
>         * passes.def (pass_all_optimizations): Move pass_object_sizes
>         after the first pass_forwprop and before pass_fre.
>
>         * gcc.dg/builtin-object-size-8.c: Un-xfail.
>         * gcc.dg/builtin-object-size-14.c: New testcase.
>         * gcc.dg/strlenopt-14gf.c: Adjust.
>         * gcc.dg/strlenopt-1f.c: Likewise.
>         * gcc.dg/strlenopt-4gf.c: Likewise.
>

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59362

-- 
H.J.