This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] add support for strnlen (PR 81384)
On Tue, Jul 10, 2018 at 4:10 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 07/10/2018 01:12 AM, Richard Biener wrote:
> > On Mon, Jul 9, 2018 at 11:26 PM Martin Sebor <msebor@gmail.com> wrote:
> >>
> >> On 07/09/2018 08:36 AM, Aldy Hernandez wrote:
> >>> { dg-do run }
> >>> { do-options "-O2 -fno-tree-strlen" } */
> >>>
> >>> ^^^^ I don't think this is doing anything.
> >>>
> >>> If you look at the test run you can see that -fno-tree-strlen is never
> >>> passed (I think you actually mean -fno-optimize-strlen for that
> >>> matter). Also, the builtins.exp harness runs your test for an
> >>> assortment of other flags, not just -O2.
> >>
> >> I didn't know the harness ignores dg-options specified in these
> >> tests. That's surprising and feels like a bug in the harness
> >> not to complain about it. The purpose of the test is to verify
> >> that the strnlen expansion in builtins.c does the right thing
> >> and it deliberately tries to disable the earlier strlen
> >> optimizations to make sure the expansion in builtins.c is fully
> >> exercised. By not pointing out my mistake the harness effectively
> >> let me commit a change without making sure it's thoroughly tested
> >> (I tested it manually before committing the patch but things could
> >> regress without us noticing). I'll look into fixing this somehow.
> >>
> >>>
> >>> This test is failing on my range branch for -Og, because
> >>> expand_builtin_strnlen() needs range info:
> >>>
> >>> + wide_int min, max;
> >>> + enum value_range_type rng = get_range_info (bound, &min, &max);
> >>> + if (rng != VR_RANGE)
> >>> + return NULL_RTX;
> >>>
> >>> but interestingly enough, it seems to be calculated in the sprintf
> >>> pass as part of the DOM walk:
> >>>
> >>> /* First record ranges generated by this statement. */
> >>> evrp_range_analyzer.record_ranges_from_stmt (stmt, false);
> >>>
> >>> It feels wrong that the sprintf warning pass is generating range info
> >>> that you may later depend on at rtl expansion time (and for a totally
> >>> unrelated thing-- strlen expansion).
> >>
> >> Any pass that records ranges for statements will have this
> >> effect. The sprintf pass seems to be the first one to make
> >> use of this utility (and it's not just a warning pass but also
> >> an optimization pass) but it would be a shame to put it off
> >> limits to warning-only passes only because it happens to set
> >> ranges.
> >
> > As you noted elsewhere warning options shouldn't change code-generation.
> > This means that ranges may not be set to the IL in case they are only
> > computed when a warning option is enabled.
>
> I agree. That's also why I think it should be a basic service
> rather than a side-effect of tree traversal, either one done
> to implement a particular optimization, or one done by a warning.
>
> The main reason for the work Jeff put in to extracting it out
> of EVRP, if I recall correctly, was to expose more accurate
> range information to warning passes. Would setting statement
> ranges make sense as part of EVRP (or some other similar pass)?
> If not, the only way to conform to the policy that I can think
> of is to have warning-only passes that need it make
> the traversal and call record_ranges regardless of whether or
> not the warning itself is enabled. That would seem needlessly
> inefficient, even if the code implementing the warning itself
> were disabled.
Well, simply not set range-info on SSA names but use the
lattice values? Should be easy to key that to a flag.
Richard.
>
> Martin