On 8/25/21 10:14 AM, Andrew MacLeod wrote:
> On 8/25/21 11:20 AM, Martin Sebor wrote:
>> Ping: Andrew, did I answer your questions?  Do you (or anyone else)
>> have any other comments on the latest patch below?
>>
>> https://gcc.gnu.org/pipermail/gcc-patches/2021-August/577865.html
> 
> 
> I wasn't attempting to block it, its outside my review purview..
> 
> I was merely commenting that you should not need a pointer to a 
> range_query at all anymore
> 
> 
>>
>>>
>>>>
>>>> Are you planning to transition to using the get_range_query() 
>>>> interface instead of keeping a range_query pointer in the 
>>>> pointer_query class?
>>>
>>> This pass and to a smaller extent the pointer_query class that's
>>> used by it and the strlen pass are still a work in progress.
>>> I also still need to convert the strlen pass to use Ranger and
>>> I expect it will take some changes to pointer_query.  So at that
>>> point, if going through get_range_query (cfun) everywhere is what
>>> you recommend, I'm happy to do it.
>>>
> absolutely. you should not need to even know whether you have a ranger 
> instance running or not. get_range_query will give you whichever is 
> active, and there is ALWAYS something active.. defaulting to the global 
> version.
> 
> the code in get_range() seems to be the end of the call chain which uses 
> the pointer and should be consolidated to something much simpler
> 
>       if (rvals && stmt)
>          {
>            if (!rvals->range_of_expr (vr, val, stmt))
>              return NULL_TREE;
> 
>          <...>
> 
>        // ?? This entire function should use get_range_query or
>     get_global_range_query (),
>        // instead of doing something different for RVALS and global ranges.
> 
>        if (!get_global_range_query ()->range_of_expr (vr, val) ||
>     vr.undefined_p ())
>          return NULL_TREE;
> 
> 
> This entire section can boil down to something like
> 
> if (!get_range_query (cfun)->range_of_expr (vr, val, stmt))
>    return NULL;

So get_range_query (cfun) will never be null?  And no special handling
of INTEGER_CST is needed, or checking for SSA_NAME.  Nice.  Attached
is another revision of the same patch with this function simplified.

(FYI: I expect the whole function to eventually go away, and to make
other similar simplifications, but I'm not there yet.)

>>> PS There has been an effort to get rid of global variables from GCC,
>>> or, as the first step, to avoid accessing them directly(*).  If and
>>> when that happens, it seems like each pass will have to store either
>>> the ranger instance as a member (directly or indirectly, via a member
>>> of a class that stores it) or the function passed to pass::execute()
>>> if it wants to access either.
>>>
>>> [*] https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573975.html
>>> The patch at the link above wasn't approved but IIUC removing globals
>>> from GCC is still a goal.
>>
> I have no idea what direction that is going, but the net effect will be 
> the same in the end.  You'll be calling get_range_query() with either 
> the function pointer, or the pass pointer, or something.. but you should 
> never need to pass around  a pointer to either a ranger or range-query.  
> As I said earlier, this will likely be a pass property and accessed thru 
> the pass eventually... but until then, use what we have.. cfun.  It'll 
> be trivial to transition down the road to whatever the ultimate solution is.

Okay, I'll do that in my future changes.

Martin