Re: [PATCH, 1/6] Simplify constraint handling

[Richard Biener <rguenther@suse.de>]

On Wed, 28 Oct 2015, Tom de Vries wrote:

> On 28/10/15 16:35, Richard Biener wrote:
> > On Tue, 27 Oct 2015, Tom de Vries wrote:
> > 
> > > On 27/10/15 13:24, Tom de Vries wrote:
> > > > Thinking it over a bit more, I realized the constraint handling started
> > > > to be messy. I've reworked the patch series to simplify that first.
> > > > 
> > > >        1    Simplify constraint handling
> > > >        2    Rename make_restrict_var_constraints to
> > > > make_param_constraints
> > > >        3    Add recursion to make_param_constraints
> > > >        4    Add handle_param parameter to create_variable_info_for_1
> > > >        5    Handle recursive restrict pointer in
> > > > create_variable_info_for_1
> > > >        6    Handle restrict struct fields recursively
> > > > 
> > > > Currently doing bootstrap and regtest on x86_64.
> > > > 
> > > > I'll repost the patch series in reply to this message.
> > > 
> > > This patch gets rid of this bit of code in intra_create_variable_infos:
> > > ...
> > >        if (restrict_pointer_p)
> > > 	make_constraint_from_global_restrict (p, "PARM_RESTRICT");
> > >        else
> > > ..
> > > 
> > > I already proposed to remove it here (
> > > https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02426.html ) but there is a
> > > problem with that approach: It can happen that restrict_pointer_p is true,
> > > but
> > > p->only_restrict_pointers is false. This happens with fipa-pta, when
> > > create_function_info_for created a varinfo for the parameter before
> > > intra_create_variable_infos was called.
> > > 
> > > The patch handles that case now by setting p->only_restrict_pointers.
> > 
> > Hmm, but ... restrict only has an effect in non-IPA mode.
> 
> Indeed, I also realized that by now.
> 
> I wrote attached patch to make that explicit and simplify fipa-pta.
> 
> OK for trunk if bootstrap and reg-test succeeds?

I don't see the patch simplifies anything but only removes spurious
settings by adding IMHO redundant checks.

> > That we
> > use intra_create_variable_infos in IPA mode is only done for correctness
> > (to set up incoming NONLOCAL) for functions we do not see all callers of.
> > 
> 
> Right.
> 
> > Maybe we should fix that (in intra_create_variable_infos properly
> > add constraints from NONLOCAL for all such functions).
> > 
> 
> Aren't we are adding those constraints currently in
> intra_create_variable_infos? Maybe you meant 'in ipa_pta_execute'?

I meant create_function_info_for.  Include all the effects of

      /* For externally visible or attribute used annotated functions use
         local constraints for their arguments.
         For local functions we see all callers and thus do not need 
initial
         constraints for parameters.  */
      if (node->used_from_other_partition
          || node->externally_visible
          || node->force_output
          || node->address_taken)
        {
          intra_create_variable_infos (func);

          /* We also need to make function return values escape.  Nothing
             escapes by returning from main though.  */
          if (!MAIN_NAME_P (DECL_NAME (node->decl)))
            {
              varinfo_t fi, rvi;
              fi = lookup_vi_for_tree (node->decl);
              rvi = first_vi_for_offset (fi, fi_result);
              if (rvi && rvi->offset == fi_result)
                {
                  struct constraint_expr includes;
                  struct constraint_expr var;
                  includes.var = escaped_id;
                  includes.offset = 0;
                  includes.type = SCALAR;
                  var.var = rvi->id;
                  var.offset = 0;
                  var.type = SCALAR;
                  process_constraint (new_constraint (includes, var));
                }
            }
        }

in it (just pass it a flag on whether the function is non-local).
The effect of intra_create_variable_infos above was supposed to
add a constraint from NONLOCAL on all vars (but the return value
which instead needs to escape as done above).

Richard.

> Thanks,
> - Tom
> 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)