On 07/10/2018 08:25 AM, Richard Biener wrote: > On Tue, Jul 10, 2018 at 4:10 PM Martin Sebor <mse...@gmail.com> wrote: >> >> On 07/10/2018 01:12 AM, Richard Biener wrote: >>> On Mon, Jul 9, 2018 at 11:26 PM Martin Sebor <mse...@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. Right. That was essentially my suggestion. For a client that uses EVRP analysis to drive warnings, they do not mirror results into the global range info we attach to SSA_NAMEs. An optimization pass which utilizes EVRP can (of course) set the global range info.
THe sprintf bits are a bit tricky as it's a mix of warning and optimization, but I think there's enough separation that we can arrange to do the right thing. Since I introduced EVRP into the sprintf bits, I'm happy to own fixing this up. I just wanted to get general agreement on the approach. jeff