diff mbox

[GOOGLE] More strict checking for call args

Message ID CAO2gOZUZ0xTXoMPmLyhsCRkyFQehu7A7EiuqS1E40hBvd8yvxQ@mail.gmail.com
State New
Headers show

Commit Message

Dehao Chen June 4, 2013, 12:55 a.m. UTC 
Hi,

This patch was committed to google branch. But I think it is of
general interest. So is it ok for trunk?

Thanks,
Dehao

gcc/ChangeLog:

2013-06-03  Dehao Chen  <dehao@google.com>

*gimple-low.c (gimple_check_call_args): Restrict the call_arg check to
contain same number of args.

Comments

Richard Biener June 4, 2013, 10:56 a.m. UTC | #1 
On Tue, Jun 4, 2013 at 2:55 AM, Dehao Chen <dehao@google.com> wrote:
> Hi,
>
> This patch was committed to google branch. But I think it is of
> general interest. So is it ok for trunk?
>
> Thanks,
> Dehao
>
> gcc/ChangeLog:
>
> 2013-06-03  Dehao Chen  <dehao@google.com>
>
> *gimple-low.c (gimple_check_call_args): Restrict the call_arg check to
> contain same number of args.
>
> Index: gcc/gimple-low.c
> ===================================================================
> --- gcc/gimple-low.c (revision 199570)
> +++ gcc/gimple-low.c (working copy)
> @@ -243,6 +243,8 @@ gimple_check_call_args (gimple stmt, tree fndecl)
>    && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
>              return false;
>   }
> +      if (p != NULL)
> + return false;

Please add a comment here, like

         /* Not enough parameters to the function call.  */
         if (p != NULL)
           return false;

note that I believe we can deal with this situation just fine during inlining,
we just leave the parameters uninitialized.

So - why do you think the test is a good idea?  The whole function should
ideally be not necessary and is just there to avoid situations we cannot
deal with during inlining.

Richard.

>      }
>    else if (parms)
>      {
Dehao Chen June 4, 2013, 3:07 p.m. UTC | #2 
On Tue, Jun 4, 2013 at 3:56 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Tue, Jun 4, 2013 at 2:55 AM, Dehao Chen <dehao@google.com> wrote:
> > Hi,
> >
> > This patch was committed to google branch. But I think it is of
> > general interest. So is it ok for trunk?
> >
> > Thanks,
> > Dehao
> >
> > gcc/ChangeLog:
> >
> > 2013-06-03  Dehao Chen  <dehao@google.com>
> >
> > *gimple-low.c (gimple_check_call_args): Restrict the call_arg check to
> > contain same number of args.
> >
> > Index: gcc/gimple-low.c
> > ===================================================================
> > --- gcc/gimple-low.c (revision 199570)
> > +++ gcc/gimple-low.c (working copy)
> > @@ -243,6 +243,8 @@ gimple_check_call_args (gimple stmt, tree fndecl)
> >    && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
> >              return false;
> >   }
> > +      if (p != NULL)
> > + return false;
>
> Please add a comment here, like
>
>          /* Not enough parameters to the function call.  */
>          if (p != NULL)
>            return false;
>
> note that I believe we can deal with this situation just fine during inlining,
> we just leave the parameters uninitialized.
>
> So - why do you think the test is a good idea?  The whole function should
> ideally be not necessary and is just there to avoid situations we cannot
> deal with during inlining.

This check is necessary when profile does not match. E.g. if an
indirect call target profile is targeting to an incorrect callee, this
patch can make sure it's verified before actually transforming code.
This could happen is you use an out-of-date profile to optimize for
new code.

Dehao



>
> Richard.
>
> >      }
> >    else if (parms)
> >      {
Xinliang David Li June 4, 2013, 3:59 p.m. UTC | #3 
Richard's question is that inlining should deal with extra arguments
just fine -- those paths (the insane profile case) won't be executed
anyway. Do you have a case showing otherwise (i.e., the mismatch
upsets the compiler?)

David


On Tue, Jun 4, 2013 at 8:07 AM, Dehao Chen <dehao@google.com> wrote:
> On Tue, Jun 4, 2013 at 3:56 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>>
>> On Tue, Jun 4, 2013 at 2:55 AM, Dehao Chen <dehao@google.com> wrote:
>> > Hi,
>> >
>> > This patch was committed to google branch. But I think it is of
>> > general interest. So is it ok for trunk?
>> >
>> > Thanks,
>> > Dehao
>> >
>> > gcc/ChangeLog:
>> >
>> > 2013-06-03  Dehao Chen  <dehao@google.com>
>> >
>> > *gimple-low.c (gimple_check_call_args): Restrict the call_arg check to
>> > contain same number of args.
>> >
>> > Index: gcc/gimple-low.c
>> > ===================================================================
>> > --- gcc/gimple-low.c (revision 199570)
>> > +++ gcc/gimple-low.c (working copy)
>> > @@ -243,6 +243,8 @@ gimple_check_call_args (gimple stmt, tree fndecl)
>> >    && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
>> >              return false;
>> >   }
>> > +      if (p != NULL)
>> > + return false;
>>
>> Please add a comment here, like
>>
>>          /* Not enough parameters to the function call.  */
>>          if (p != NULL)
>>            return false;
>>
>> note that I believe we can deal with this situation just fine during inlining,
>> we just leave the parameters uninitialized.
>>
>> So - why do you think the test is a good idea?  The whole function should
>> ideally be not necessary and is just there to avoid situations we cannot
>> deal with during inlining.
>
> This check is necessary when profile does not match. E.g. if an
> indirect call target profile is targeting to an incorrect callee, this
> patch can make sure it's verified before actually transforming code.
> This could happen is you use an out-of-date profile to optimize for
> new code.
>
> Dehao
>
>
>
>>
>> Richard.
>>
>> >      }
>> >    else if (parms)
>> >      {
Dehao Chen June 5, 2013, 12:19 a.m. UTC | #4 
attached is a testcase that would cause problem when source has changed:

$ g++ test.cc -O2 -fprofile-generate -DOLD
$ ./a.out
$ g++ test.cc -O2 -fprofile-use
test.cc:34:1: internal compiler error: in operator[], at vec.h:815
 }
 ^
0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
../../gcc/vec.h:815
0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
../../gcc/vec.h:1244
0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
../../gcc/vec.h:815
0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
../../gcc/vec.h:1244
0xf24464 ipa_get_indirect_edge_target_1
../../gcc/ipa-cp.c:1535
0x971b9a estimate_edge_devirt_benefit
../../gcc/ipa-inline-analysis.c:2757
0x973f59 estimate_edge_size_and_time
../../gcc/ipa-inline-analysis.c:2789
0x973f59 estimate_calls_size_and_time
../../gcc/ipa-inline-analysis.c:2842
0x97429f estimate_node_size_and_time
../../gcc/ipa-inline-analysis.c:2929
0x976077 do_estimate_edge_size(cgraph_edge*)
../../gcc/ipa-inline-analysis.c:3472
0x97614f estimate_edge_size
../../gcc/ipa-inline.h:274
0x97614f estimate_edge_growth
../../gcc/ipa-inline.h:286
0x97614f do_estimate_growth_1
../../gcc/ipa-inline-analysis.c:3582
0x7e41df cgraph_for_node_and_aliases
../../gcc/cgraph.c:1777
0x976675 do_estimate_growth(cgraph_node*)
../../gcc/ipa-inline-analysis.c:3596
0xf314ea estimate_growth
../../gcc/ipa-inline.h:261
0xf314ea inline_small_functions
../../gcc/ipa-inline.c:1432
0xf314ea ipa_inline
../../gcc/ipa-inline.c:1797
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <http://gcc.gnu.org/bugs.html> for instructions.
Richard Biener June 5, 2013, 8:31 a.m. UTC | #5 
On Wed, Jun 5, 2013 at 2:19 AM, Dehao Chen <dehao@google.com> wrote:
> attached is a testcase that would cause problem when source has changed:
>
> $ g++ test.cc -O2 -fprofile-generate -DOLD
> $ ./a.out
> $ g++ test.cc -O2 -fprofile-use
> test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>  }
>  ^
> 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
> ../../gcc/vec.h:815
> 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
> ../../gcc/vec.h:1244
> 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
> ../../gcc/vec.h:815
> 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
> ../../gcc/vec.h:1244
> 0xf24464 ipa_get_indirect_edge_target_1
> ../../gcc/ipa-cp.c:1535

This use needs to be properly guarded.  We can perfectly well have
mismatching fndecl nodes in gimple calls.  If we start with

void fn(int, int, int);

...
void (*x)(float, double, struct X, int) = fn;
(*x)(1., 2., {}, 1);

the GIMPLE_CALL receives the function type effective for the call
from the source (gimple_call_fntype).  Then CCP happily propagates the
'fn' decl and we end up with

  fn (1., 2., {}, 1);

that is, gimple_call_fndecl is 'fn' but gimple_call_fntype is still
void (*x)(float, double, struct X, int)!

So the solution is not to fix the argument verification predicate but to make
code aware of the fact that for the call statement gimple_call_fntype is
relevant for what is a valid call (that's also what is verified against in
verify_stmts) even though the ultimate called function-decl 'fn' has a
different prototype.  Thus any code propagating from a call site to
the callee has to deal with mismatches.

Richard.

> 0x971b9a estimate_edge_devirt_benefit
> ../../gcc/ipa-inline-analysis.c:2757
> 0x973f59 estimate_edge_size_and_time
> ../../gcc/ipa-inline-analysis.c:2789
> 0x973f59 estimate_calls_size_and_time
> ../../gcc/ipa-inline-analysis.c:2842
> 0x97429f estimate_node_size_and_time
> ../../gcc/ipa-inline-analysis.c:2929
> 0x976077 do_estimate_edge_size(cgraph_edge*)
> ../../gcc/ipa-inline-analysis.c:3472
> 0x97614f estimate_edge_size
> ../../gcc/ipa-inline.h:274
> 0x97614f estimate_edge_growth
> ../../gcc/ipa-inline.h:286
> 0x97614f do_estimate_growth_1
> ../../gcc/ipa-inline-analysis.c:3582
> 0x7e41df cgraph_for_node_and_aliases
> ../../gcc/cgraph.c:1777
> 0x976675 do_estimate_growth(cgraph_node*)
> ../../gcc/ipa-inline-analysis.c:3596
> 0xf314ea estimate_growth
> ../../gcc/ipa-inline.h:261
> 0xf314ea inline_small_functions
> ../../gcc/ipa-inline.c:1432
> 0xf314ea ipa_inline
> ../../gcc/ipa-inline.c:1797
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See <http://gcc.gnu.org/bugs.html> for instructions.
Xinliang David Li June 5, 2013, 4:11 p.m. UTC | #6 
Right, except that in the context of FDO/autoFDO, where this happens
the most (note in FDO case, it can happen with fresh profile too for
multi-threaded programs), it is not that important to handle -- the
mismatch path will never be executed, so why bother to inline and
bloat the code for it?

if (fptr_new == func_old) {
      func_old (ptr);             <--- do not want to inline.
}
else
   fptr_new(ptr);

David


On Wed, Jun 5, 2013 at 1:31 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Wed, Jun 5, 2013 at 2:19 AM, Dehao Chen <dehao@google.com> wrote:
>> attached is a testcase that would cause problem when source has changed:
>>
>> $ g++ test.cc -O2 -fprofile-generate -DOLD
>> $ ./a.out
>> $ g++ test.cc -O2 -fprofile-use
>> test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>  }
>>  ^
>> 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>> ../../gcc/vec.h:815
>> 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>> ../../gcc/vec.h:1244
>> 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>> ../../gcc/vec.h:815
>> 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>> ../../gcc/vec.h:1244
>> 0xf24464 ipa_get_indirect_edge_target_1
>> ../../gcc/ipa-cp.c:1535
>
> This use needs to be properly guarded.  We can perfectly well have
> mismatching fndecl nodes in gimple calls.  If we start with
>
> void fn(int, int, int);
>
> ...
> void (*x)(float, double, struct X, int) = fn;
> (*x)(1., 2., {}, 1);
>
> the GIMPLE_CALL receives the function type effective for the call
> from the source (gimple_call_fntype).  Then CCP happily propagates the
> 'fn' decl and we end up with
>
>   fn (1., 2., {}, 1);
>
> that is, gimple_call_fndecl is 'fn' but gimple_call_fntype is still
> void (*x)(float, double, struct X, int)!
>
> So the solution is not to fix the argument verification predicate but to make
> code aware of the fact that for the call statement gimple_call_fntype is
> relevant for what is a valid call (that's also what is verified against in
> verify_stmts) even though the ultimate called function-decl 'fn' has a
> different prototype.  Thus any code propagating from a call site to
> the callee has to deal with mismatches.
>
> Richard.
>
>> 0x971b9a estimate_edge_devirt_benefit
>> ../../gcc/ipa-inline-analysis.c:2757
>> 0x973f59 estimate_edge_size_and_time
>> ../../gcc/ipa-inline-analysis.c:2789
>> 0x973f59 estimate_calls_size_and_time
>> ../../gcc/ipa-inline-analysis.c:2842
>> 0x97429f estimate_node_size_and_time
>> ../../gcc/ipa-inline-analysis.c:2929
>> 0x976077 do_estimate_edge_size(cgraph_edge*)
>> ../../gcc/ipa-inline-analysis.c:3472
>> 0x97614f estimate_edge_size
>> ../../gcc/ipa-inline.h:274
>> 0x97614f estimate_edge_growth
>> ../../gcc/ipa-inline.h:286
>> 0x97614f do_estimate_growth_1
>> ../../gcc/ipa-inline-analysis.c:3582
>> 0x7e41df cgraph_for_node_and_aliases
>> ../../gcc/cgraph.c:1777
>> 0x976675 do_estimate_growth(cgraph_node*)
>> ../../gcc/ipa-inline-analysis.c:3596
>> 0xf314ea estimate_growth
>> ../../gcc/ipa-inline.h:261
>> 0xf314ea inline_small_functions
>> ../../gcc/ipa-inline.c:1432
>> 0xf314ea ipa_inline
>> ../../gcc/ipa-inline.c:1797
>> Please submit a full bug report,
>> with preprocessed source if appropriate.
>> Please include the complete backtrace with any bug report.
>> See <http://gcc.gnu.org/bugs.html> for instructions.
Richard Biener June 6, 2013, 8:19 a.m. UTC | #7 
On Wed, Jun 5, 2013 at 6:11 PM, Xinliang David Li <davidxl@google.com> wrote:
> Right, except that in the context of FDO/autoFDO, where this happens
> the most (note in FDO case, it can happen with fresh profile too for
> multi-threaded programs), it is not that important to handle -- the
> mismatch path will never be executed, so why bother to inline and
> bloat the code for it?

It's about being able to IPA-CP through such calls.  Consider

void foo (int);
void bar (int i, float)
{
  foo (i);
}
void foobar ()
{
  bar (1);  // mismatched # of arguments but we can see foo() is
called with constant 1
}

that's important if we can prove that all calls to foo () are called
with a constant 1
argument for example and thus we can replace it without cloning.

If the compatibility predicate guards even the IPA-CP case (not just the
inlining itself) then the predicate is used in a bogus way.

So your symptom is not properly fixed with the patch but papered over.

Richard.

> if (fptr_new == func_old) {
>       func_old (ptr);             <--- do not want to inline.
> }
> else
>    fptr_new(ptr);
>
> David
>
>
> On Wed, Jun 5, 2013 at 1:31 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Wed, Jun 5, 2013 at 2:19 AM, Dehao Chen <dehao@google.com> wrote:
>>> attached is a testcase that would cause problem when source has changed:
>>>
>>> $ g++ test.cc -O2 -fprofile-generate -DOLD
>>> $ ./a.out
>>> $ g++ test.cc -O2 -fprofile-use
>>> test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>>  }
>>>  ^
>>> 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>> ../../gcc/vec.h:815
>>> 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>> ../../gcc/vec.h:1244
>>> 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>> ../../gcc/vec.h:815
>>> 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>> ../../gcc/vec.h:1244
>>> 0xf24464 ipa_get_indirect_edge_target_1
>>> ../../gcc/ipa-cp.c:1535
>>
>> This use needs to be properly guarded.  We can perfectly well have
>> mismatching fndecl nodes in gimple calls.  If we start with
>>
>> void fn(int, int, int);
>>
>> ...
>> void (*x)(float, double, struct X, int) = fn;
>> (*x)(1., 2., {}, 1);
>>
>> the GIMPLE_CALL receives the function type effective for the call
>> from the source (gimple_call_fntype).  Then CCP happily propagates the
>> 'fn' decl and we end up with
>>
>>   fn (1., 2., {}, 1);
>>
>> that is, gimple_call_fndecl is 'fn' but gimple_call_fntype is still
>> void (*x)(float, double, struct X, int)!
>>
>> So the solution is not to fix the argument verification predicate but to make
>> code aware of the fact that for the call statement gimple_call_fntype is
>> relevant for what is a valid call (that's also what is verified against in
>> verify_stmts) even though the ultimate called function-decl 'fn' has a
>> different prototype.  Thus any code propagating from a call site to
>> the callee has to deal with mismatches.
>>
>> Richard.
>>
>>> 0x971b9a estimate_edge_devirt_benefit
>>> ../../gcc/ipa-inline-analysis.c:2757
>>> 0x973f59 estimate_edge_size_and_time
>>> ../../gcc/ipa-inline-analysis.c:2789
>>> 0x973f59 estimate_calls_size_and_time
>>> ../../gcc/ipa-inline-analysis.c:2842
>>> 0x97429f estimate_node_size_and_time
>>> ../../gcc/ipa-inline-analysis.c:2929
>>> 0x976077 do_estimate_edge_size(cgraph_edge*)
>>> ../../gcc/ipa-inline-analysis.c:3472
>>> 0x97614f estimate_edge_size
>>> ../../gcc/ipa-inline.h:274
>>> 0x97614f estimate_edge_growth
>>> ../../gcc/ipa-inline.h:286
>>> 0x97614f do_estimate_growth_1
>>> ../../gcc/ipa-inline-analysis.c:3582
>>> 0x7e41df cgraph_for_node_and_aliases
>>> ../../gcc/cgraph.c:1777
>>> 0x976675 do_estimate_growth(cgraph_node*)
>>> ../../gcc/ipa-inline-analysis.c:3596
>>> 0xf314ea estimate_growth
>>> ../../gcc/ipa-inline.h:261
>>> 0xf314ea inline_small_functions
>>> ../../gcc/ipa-inline.c:1432
>>> 0xf314ea ipa_inline
>>> ../../gcc/ipa-inline.c:1797
>>> Please submit a full bug report,
>>> with preprocessed source if appropriate.
>>> Please include the complete backtrace with any bug report.
>>> See <http://gcc.gnu.org/bugs.html> for instructions.
diff mbox

Patch

Index: gcc/gimple-low.c
===================================================================
--- gcc/gimple-low.c (revision 199570)
+++ gcc/gimple-low.c (working copy)
@@ -243,6 +243,8 @@  gimple_check_call_args (gimple stmt, tree fndecl)
   && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
             return false;
  }
+      if (p != NULL)
+ return false;
     }
   else if (parms)
     {