Re: [patch] convert -Wrestrict pass to ranger

[Andrew MacLeod <amacleod@redhat.com>]

On 10/5/20 4:16 PM, Martin Sebor wrote:
> On 10/5/20 8:50 AM, Aldy Hernandez via Gcc-patches wrote:
>> [Martin, as the original author of this pass, do you have any concerns?]
>>
>
>> @@ -1270,7 +1271,21 @@ get_size_range (tree exp, tree range[2], bool 
>> allow_zero /* = false */)
>>     enum value_range_kind range_type;
>>
>>     if (integral)
>> -    range_type = determine_value_range (exp, &min, &max);
>> +    {
>> +      if (query)
>> +    {
>> +      value_range vr;
>> +      gcc_assert (TREE_CODE (exp) == SSA_NAME
>> +              || TREE_CODE (exp) == INTEGER_CST);
>> +      gcc_assert (query->range_of_expr (vr, exp, stmt));
>
> Will the call to the function in the assert above not be eliminated
> if the assert is turned into a no-op?  If it can't happen (it looks
> like it shouldn't anymore), it would still be nice to break it out
> of the macro.  Those of us used to the semantics of the C standard
> assert macro might wonder.
>
gcc_assert contents will not be eliminated in a release compiler, only 
the actual check of the return value.    The body of the assert will 
continue to be executed.

This exists because if we were to try to check the return value, we'd 
have to do something like
   bool ret = range_of_expr (...)
   gcc_checking_assert (ret);

and when the checking assert goes away, we're left with an unused 
variable 'ret' warning.   the gcc_assert ()  resolves that issue.

I'm not a huge fan of them, but they do serve a purpose and seem better 
than the alternatives :-P

The first assert should however be a gcc_checking_assert since its just 
a check.. and then that will go away in a release compiler.

>> -/* Execute the pass for function FUN, walking in dominator order.  */
>> -
>>   unsigned
>>   pass_wrestrict::execute (function *fun)
>>   {
>> -  calculate_dominance_info (CDI_DOMINATORS);
>> -
>> -  wrestrict_dom_walker walker;
>> -  walker.walk (ENTRY_BLOCK_PTR_FOR_FN (fun));
>> +  gimple_ranger ranger;
>> +  basic_block bb;
>> +  FOR_EACH_BB_FN (bb, fun)
>> +    wrestrict_walk (&ranger, bb);
>>
>>     return 0;
>>   }
>> @@ -159,11 +144,14 @@ public:
>>        only the destination reference is.  */
>>     bool strbounded_p;
>>
>> -  builtin_memref (tree, tree);
>> +  builtin_memref (range_query *, gimple *, tree, tree);
>>
>>     tree offset_out_of_bounds (int, offset_int[3]) const;
>>
>>   private:
>> +  gimple *stmt;
>> +
>> +  range_query *query;
>
> Also please add a comment above STMT to make it clear it's the call
> statement to the builtin.
>
> For QUERY, I'm not sure adding a member to every class that needs
> to compute ranges is the right design.  At the same time, adding
> an argument to every function that computes ranges isn't great
> either.  It seems like there should be one shared ranger instance
> that could be used by all clients/passes as well as untility
> functions called from them.  It could be a global object set/pushed
> by each pass when it starts and popped when it ends, and managed by
> the ranger API.  Say a static member function:
>
>   range_query* range_query::instance ();
>   range_query* range_query::push_instance (range_query*);
>   range_query* range_query::pop_instance ();
>
> As some background, when I wrote the builtin_access access
> I envisioned using it as a general-purpose class in other similar
> contexts.  That hasn't quite happened yet but there is a class
> kind of like it that might eventually end up taking the place of
> builtin_access.  It's access_ref in builtins.h.  And while neither
> class crates a lot of instances so far, I'm about to post a patch
> that does create one or two instances of access_ref per SSA_NAME
> of pointer type.  Having an extra member in each instance just
> to gain access to an API would be excessive.
>
> I'm not saying all this as an objection to the change but more
> as something to think about going forward.

Long term, I would expect we might have some sort of general access... 
probably thru cfun.     so any pass/routines would just ask for
     RANGE_INFO (cfun)->range_of_expr()
The default would be a general value_range implementation which probably 
implements something like determine_value_range_1 ().. and if a pass 
wants to use a ranger, then it could register a ranger, and when its 
done delete it.  and it would just work for everyone everywhere.

but we're not there yet, and we havent worked out the details :-) for 
the moment, passes need to figure out themselves how to access the 
ranger they create if they want one.   They had to manage a 
range_analyzer before if the used anything other than global ranges, so 
that is nothing new.