On Mon, Aug 30, 2021 at 4:27 PM Martin Sebor <mse...@gmail.com> wrote: > > Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578135.html > > Are there any further comments on the patch? > > Richard, you were kind enough to review the first two patches in this > series. Would you mind doing the same for this one? It continues in > the same vein.
OK. Richard. > Martin > > On 8/25/21 3:26 PM, Martin Sebor wrote: > > 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 >
