| Message ID | CA+=Sn1nh-TNmTzbu1CoQGurRwm9eHNos7y4R9Esg6ans_GCDqQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Sep 21, 2016 at 9:13 AM, Andrew Pinski <pinskia@gmail.com> wrote: > Hi, > While looking through bug reports, I noticed someone had reported > that LTO caused the vectorizer not to kick in. Originally it was not > obvious why it would change the behavior since this was a simple > program with nothing out of the ordinary. Well it turned out paths > leading to the exit(0); at the end of main was being marked as cold > and in the LTO case the funciton (which had loop which should have > been vectorized) was considered local and being marked as cold as it > was only called now from the path leading to the exit(0); Since > exit(0); is considered a normal exit path, there is no reason to mark > it as being as a cold path; let the other predicate handle it. > > So this patch changes the predicate for exit(0) not to mark the paths > leading up to that function call as being cold. Note this patch only > disables that when the argument is a literal zero so if we a PHI node > which contains the exit value, we still cause those paths to be > considered cold; this will be for another patch. Hmm, it also doesn't work for main calling a function not returning but exiting with exit (0) (it will be discovered as noreturn). I don't think that treating exit (0) as generally not terminating a cold code sequence is good either. Predictions are always hard I guess ... But the thing to improve is maybe the different handling of main with respect to the guessed profile -- this is something that should not happen inconsistently between LTO / non-LTO as main is special in all cases. Honza? Richard. > OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions. > Also tested it with SPEC CPU 2006 with no performance regressions. > > Thanks, > Andrew Pinski > > ChangeLog: > * predict.c (is_exit_with_zero_arg): New function. > (tree_bb_level_predictions): Don't consider paths leading to exit(0) > as nottaken.
On Wed, Sep 21, 2016 at 4:32 PM, Richard Biener <richard.guenther@gmail.com> wrote: > On Wed, Sep 21, 2016 at 9:13 AM, Andrew Pinski <pinskia@gmail.com> wrote: >> Hi, >> While looking through bug reports, I noticed someone had reported >> that LTO caused the vectorizer not to kick in. Originally it was not >> obvious why it would change the behavior since this was a simple >> program with nothing out of the ordinary. Well it turned out paths >> leading to the exit(0); at the end of main was being marked as cold >> and in the LTO case the funciton (which had loop which should have >> been vectorized) was considered local and being marked as cold as it >> was only called now from the path leading to the exit(0); Since >> exit(0); is considered a normal exit path, there is no reason to mark >> it as being as a cold path; let the other predicate handle it. >> >> So this patch changes the predicate for exit(0) not to mark the paths >> leading up to that function call as being cold. Note this patch only >> disables that when the argument is a literal zero so if we a PHI node >> which contains the exit value, we still cause those paths to be >> considered cold; this will be for another patch. > > Hmm, it also doesn't work for main calling a function not returning but exiting > with exit (0) (it will be discovered as noreturn). > > I don't think that treating exit (0) as generally not terminating a cold code > sequence is good either. > > Predictions are always hard I guess ... > > But the thing to improve is maybe the different handling of main with > respect to the guessed profile -- this is something that should not happen > inconsistently between LTO / non-LTO as main is special in all cases. Honza? Maybe I did not explain it very well but what is happening is a function which is only called in the cold path is marked as cold only in the LTO case as it is extern call.. Basically with LTO, the function becomes local so it can be marked as cold while without LTO, it cannot. Thanks, Andrew > > Richard. > >> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions. >> Also tested it with SPEC CPU 2006 with no performance regressions. >> >> Thanks, >> Andrew Pinski >> >> ChangeLog: >> * predict.c (is_exit_with_zero_arg): New function. >> (tree_bb_level_predictions): Don't consider paths leading to exit(0) >> as nottaken.
On Wed, Sep 21, 2016 at 10:45 AM, Andrew Pinski <pinskia@gmail.com> wrote: > On Wed, Sep 21, 2016 at 4:32 PM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Wed, Sep 21, 2016 at 9:13 AM, Andrew Pinski <pinskia@gmail.com> wrote: >>> Hi, >>> While looking through bug reports, I noticed someone had reported >>> that LTO caused the vectorizer not to kick in. Originally it was not >>> obvious why it would change the behavior since this was a simple >>> program with nothing out of the ordinary. Well it turned out paths >>> leading to the exit(0); at the end of main was being marked as cold >>> and in the LTO case the funciton (which had loop which should have >>> been vectorized) was considered local and being marked as cold as it >>> was only called now from the path leading to the exit(0); Since >>> exit(0); is considered a normal exit path, there is no reason to mark >>> it as being as a cold path; let the other predicate handle it. >>> >>> So this patch changes the predicate for exit(0) not to mark the paths >>> leading up to that function call as being cold. Note this patch only >>> disables that when the argument is a literal zero so if we a PHI node >>> which contains the exit value, we still cause those paths to be >>> considered cold; this will be for another patch. >> >> Hmm, it also doesn't work for main calling a function not returning but exiting >> with exit (0) (it will be discovered as noreturn). >> >> I don't think that treating exit (0) as generally not terminating a cold code >> sequence is good either. >> >> Predictions are always hard I guess ... >> >> But the thing to improve is maybe the different handling of main with >> respect to the guessed profile -- this is something that should not happen >> inconsistently between LTO / non-LTO as main is special in all cases. Honza? > > Maybe I did not explain it very well but what is happening is a > function which is only called in the cold path is marked as cold only > in the LTO case as it is extern call.. Basically with LTO, the > function becomes local so it can be marked as cold while without LTO, > it cannot. Ah, so you have foo () { loop } main() { if () { foo (); exit (0); } ... return 0; } and foo is marked cold because its only call is on the path to exit (0)? noreturn prediction is quite aggressive but it works also quite well. Given you can only detect a very minor fraction of cases (consider exit (0) being in foo itself) I'm not sure that your fix is good progress. IMHO we might want to switch from 'noreturn' to 'noreturn' + likely error which needs another attribute / auto-discovery and IPA propagation to handle this case better. Anyway, I'll leave the patch to Honza. Richard. > Thanks, > Andrew > >> >> Richard. >> >>> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions. >>> Also tested it with SPEC CPU 2006 with no performance regressions. >>> >>> Thanks, >>> Andrew Pinski >>> >>> ChangeLog: >>> * predict.c (is_exit_with_zero_arg): New function. >>> (tree_bb_level_predictions): Don't consider paths leading to exit(0) >>> as nottaken.
On Wed, Sep 21, 2016 at 1:53 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Wed, Sep 21, 2016 at 10:45 AM, Andrew Pinski <pinskia@gmail.com> wrote: >> On Wed, Sep 21, 2016 at 4:32 PM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> On Wed, Sep 21, 2016 at 9:13 AM, Andrew Pinski <pinskia@gmail.com> wrote: >>>> Hi, >>>> While looking through bug reports, I noticed someone had reported >>>> that LTO caused the vectorizer not to kick in. Originally it was not >>>> obvious why it would change the behavior since this was a simple >>>> program with nothing out of the ordinary. Well it turned out paths >>>> leading to the exit(0); at the end of main was being marked as cold >>>> and in the LTO case the funciton (which had loop which should have >>>> been vectorized) was considered local and being marked as cold as it >>>> was only called now from the path leading to the exit(0); Since >>>> exit(0); is considered a normal exit path, there is no reason to mark >>>> it as being as a cold path; let the other predicate handle it. >>>> >>>> So this patch changes the predicate for exit(0) not to mark the paths >>>> leading up to that function call as being cold. Note this patch only >>>> disables that when the argument is a literal zero so if we a PHI node >>>> which contains the exit value, we still cause those paths to be >>>> considered cold; this will be for another patch. >>> >>> Hmm, it also doesn't work for main calling a function not returning but exiting >>> with exit (0) (it will be discovered as noreturn). >>> >>> I don't think that treating exit (0) as generally not terminating a cold code >>> sequence is good either. >>> >>> Predictions are always hard I guess ... >>> >>> But the thing to improve is maybe the different handling of main with >>> respect to the guessed profile -- this is something that should not happen >>> inconsistently between LTO / non-LTO as main is special in all cases. Honza? >> >> Maybe I did not explain it very well but what is happening is a >> function which is only called in the cold path is marked as cold only >> in the LTO case as it is extern call.. Basically with LTO, the >> function becomes local so it can be marked as cold while without LTO, >> it cannot. > > Ah, so you have > > foo () { loop } > main() > { > if () > { > foo (); > exit (0); > } > ... > return 0; > } > > and foo is marked cold because its only call is on the path to exit (0)? Actually the case I have here is just: foo () { loop } int main(void) { ..... foo(); ... exit (0); } Just an exit at the end of main. Basically if we treat exit(0) as a normal function, main becomes hot again. Thanks, Andrew > > noreturn prediction is quite aggressive but it works also quite well. Given > you can only detect a very minor fraction of cases (consider exit (0) being > in foo itself) I'm not sure that your fix is good progress. IMHO we might > want to switch from 'noreturn' to 'noreturn' + likely error which needs > another attribute / auto-discovery and IPA propagation to handle this case > better. > > Anyway, I'll leave the patch to Honza. > > Richard. > >> Thanks, >> Andrew >> >>> >>> Richard. >>> >>>> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions. >>>> Also tested it with SPEC CPU 2006 with no performance regressions. >>>> >>>> Thanks, >>>> Andrew Pinski >>>> >>>> ChangeLog: >>>> * predict.c (is_exit_with_zero_arg): New function. >>>> (tree_bb_level_predictions): Don't consider paths leading to exit(0) >>>> as nottaken.
> > Ah, so you have > > > > foo () { loop } > > main() > > { > > if () > > { > > foo (); > > exit (0); > > } > > ... > > return 0; > > } > > > > and foo is marked cold because its only call is on the path to exit (0)? > > > Actually the case I have here is just: > foo () { loop } > int main(void) > { > ..... > foo(); > ... > exit (0); > } > > Just an exit at the end of main. > Basically if we treat exit(0) as a normal function, main becomes hot again. Yep, it is old noreturn predicate lazynes. Path is OK Honza > > Thanks, > Andrew > > > > > noreturn prediction is quite aggressive but it works also quite well. Given > > you can only detect a very minor fraction of cases (consider exit (0) being > > in foo itself) I'm not sure that your fix is good progress. IMHO we might > > want to switch from 'noreturn' to 'noreturn' + likely error which needs > > another attribute / auto-discovery and IPA propagation to handle this case > > better. > > > > Anyway, I'll leave the patch to Honza. > > > > Richard. > > > >> Thanks, > >> Andrew > >> > >>> > >>> Richard. > >>> > >>>> OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions. > >>>> Also tested it with SPEC CPU 2006 with no performance regressions. > >>>> > >>>> Thanks, > >>>> Andrew Pinski > >>>> > >>>> ChangeLog: > >>>> * predict.c (is_exit_with_zero_arg): New function. > >>>> (tree_bb_level_predictions): Don't consider paths leading to exit(0) > >>>> as nottaken.
Index: gcc/predict.c =================================================================== --- gcc/predict.c (revision 240299) +++ gcc/predict.c (working copy) @@ -2513,6 +2513,21 @@ } } +/* Returns TRUE if the STMT is exit(0) like statement. */ + +static bool +is_exit_with_zero_arg (const gimple *stmt) +{ + /* This is not exit, _exit or _Exit. */ + if (!gimple_call_builtin_p (stmt, BUILT_IN_EXIT) + && !gimple_call_builtin_p (stmt, BUILT_IN__EXIT) + && !gimple_call_builtin_p (stmt, BUILT_IN__EXIT2)) + return false; + + /* Argument is an interger zero. */ + return integer_zerop (gimple_call_arg (stmt, 0)); +} + /* Try to guess whether the value of return means error code. */ static enum br_predictor @@ -2639,7 +2654,9 @@ if (is_gimple_call (stmt)) { - if (gimple_call_noreturn_p (stmt) && has_return_edges) + if (gimple_call_noreturn_p (stmt) + && has_return_edges + && !is_exit_with_zero_arg (stmt)) predict_paths_leading_to (bb, PRED_NORETURN, NOT_TAKEN); decl = gimple_call_fndecl (stmt);