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

[Richard Biener <rguenther at suse dot de>]

On Thu, 29 Oct 2015, Tom de Vries wrote:

> On 29/10/15 14:12, Richard Biener wrote:
> > On Thu, 29 Oct 2015, Tom de Vries wrote:
> > 
> > > >On 29/10/15 12:13, Richard Biener wrote:
> > > > > >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?
> > > >
> > > >First, there was an error in the patch, it tested for flag_ipa_pta (so it
> > > also
> > > >affected ealias), but it was supposed to test for in_ipa mode. That is
> > > fixed
> > > >in attached version.
> > > >
> > > > > >I don't see the patch simplifies anything but only removes spurious
> > > > > >settings by adding IMHO redundant checks.
> > > >
> > > >Consider testcase:
> > > >...
> > > >int __attribute__((noinline, noclone))
> > > >foo (int *__restrict__ a, int *__restrict__ b)
> > > >{
> > > >   *a = 1;
> > > >   *b = 2;
> > > >}
> > > >
> > > >int __attribute__((noinline, noclone))
> > > >bar (int *a, int *b)
> > > >{
> > > >   foo (a, b);
> > > >}
> > > >...
> > > >
> > > >The impact of this patch in the pta dump (focusing on the constraints
> > > bit) is:
> > > >...
> > > >  Generating constraints for foo (foo)
> > > >
> > > >-foo.arg0 = &PARM_NOALIAS(20)
> > > >-PARM_NOALIAS(20) = NONLOCAL
> > > >-foo.arg1 = &PARM_NOALIAS(21)
> > > >-PARM_NOALIAS(21) = NONLOCAL
> > > >+foo.arg0 = &NONLOCAL
> > > >+foo.arg1 = &NONLOCAL
> > > >...
> > > >
> > > >That's the kind of simplifications I'm trying to achieve.
> > Yes, but as I said we should refactor things to avoid calling
> > the intra constraints generation from the IPA path.
> 
> Ah, I see.
> 
> So, like this? OK for trunk if bootstrap and reg-test succeeds?

Yes, like this.  But you miss to apply the same to the static chain,
and the varargs "rest".

Ok with that change.

Richard.