On Tue, May 29, 2018 at 4:58 PM Martin Sebor <mse...@gmail.com> wrote:
> On 05/28/2018 03:11 AM, Richard Biener wrote: > > On Fri, May 25, 2018 at 10:15 PM Martin Sebor <mse...@gmail.com> wrote: > > > >> Attached is revision 3 of the patch incorporating your > >> determine_value_range function with the requested changes. > > > > I'm somewhat torn about removing the "basic" interface on SSA names > > so can you please not change get_range_info for now and instead > > use determine_value_range in get_size_range for now? > I can do that. Can you explain why you're having second thoughts > about going this route? I've seen you recurse between both APIs, thus they call each other. That's ugly which is why I prefer to keep one of them a simple accessor to the range-info associated with an SSA name. A future enhancement for the new API would be to walk def stmts but then the API should stop at SSA names that do have range-info associated and record range-info it computed into SSA names it walked so the IL itself serves as a cache. That requires a way to see whether an SSA name has range-info rather than having get_range_info recurse into the walking machinery again. > FWIW: I've already made changes to most clients to get_range_info > to let them call the function for non-SSA arguments and have been > testing the (incremental) patch. I haven't seen a dramatic increase > in the number of successful calls to the function as a result so it > doesn't seem like it would be too much of an improvement. I did > notice that some calls end up returning a one-element range, i.e., > N, N]. Unless callers are prepared to handle such ranges this could > expose bugs or optimization opportunities. Is it worth finishing > this up? It's probably missed optimization opportunities but they might be caused by "bad" pass placement. But yes, in principle finishing this up is worthwhile - we should just keep a bare-metal get_range_info somehow. We can use an alternate name for that one for example - get_ssa_range_info for example? And adjust the setters accordingly. Or go away with a new name for the enhanced API - I like get_expr_range_info more there but it of course requires changing all affected users. If I had the choice and would do the implementation I would do this - make the enhanced API called get_expr_range_info. Richard. > Martin
