On 3 January 2018 at 18:58, Jan Hubicka <hubi...@ucw.cz> wrote: > >> diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c >> index 09ca3590039..0406d5588d2 100644 >> --- a/gcc/ipa-pure-const.c >> +++ b/gcc/ipa-pure-const.c >> @@ -910,7 +910,8 @@ malloc_candidate_p (function *fun, bool ipa) >> #define DUMP_AND_RETURN(reason) \ >> { \ >> if (dump_file && (dump_flags & TDF_DETAILS)) \ >> - fprintf (dump_file, "%s", (reason)); \ >> + fprintf (dump_file, "\n%s is not a malloc candidate, reason: %s\n", \ >> + (node->name()), (reason)); \ >> return false; \ >> } >> >> @@ -961,11 +962,14 @@ malloc_candidate_p (function *fun, bool ipa) >> for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i) >> { >> tree arg = gimple_phi_arg_def (phi, i); >> + if (arg == null_pointer_node) >> + continue; > > I think you want operand_equal_p here and also check for > flag_delete_null_pointer_checks > because otherwise 0 can be legal memory block addrss. >> + >> if (TREE_CODE (arg) != SSA_NAME) >> - DUMP_AND_RETURN("phi arg is not SSA_NAME.") >> - if (!(arg == null_pointer_node || check_retval_uses (arg, phi))) >> - DUMP_AND_RETURN("phi arg has uses outside phi" >> - " and comparisons against 0.") >> + DUMP_AND_RETURN ("phi arg is not SSA_NAME."); >> + if (!check_retval_uses (arg, phi)) >> + DUMP_AND_RETURN ("phi arg has uses outside phi" >> + " and comparisons against 0.") > > So the case of phi(0,0) is not really correctness issue just missed > optimization? > I would still add code to handle it (it is easy). Yes, I suppose it won't affect correctness, since it would be marked as TOP and after propagation it would be lowered to BOTTOM, but I added the check anyway. > > Do you also handle a case where parameter of phi is another phi? Indeed, it doesn't handle multiple-phi's as in the following test-case, good catch!
void *g (int cond1, int cond2, int cond3) { void *ret; void *a; void *b; if (cond1) a = __builtin_malloc (10); else a = __builtin_malloc (20); if (cond2) b = __builtin_malloc (30); else b = __builtin_malloc (40); if (cond3) ret = a; else ret = b; return ret; } local-pure-const dump shows the function not being marked as malloc while it should be. I will address this issue in a follow up patch. This patch check for flag_delete_null_pointer_checks and wheteher all phi args are 0 in the attached patch. Validation in progress. Does it look OK ? As Jakub pointed out for the case: void *f() { return __builtin_malloc (0); } The malloc propagation would set f() to malloc. However AFAIU, malloc(0) returns NULL (?) and the function shouldn't be marked as malloc ? Thanks, Prathamesh > > Honza >> >> gimple *arg_def = SSA_NAME_DEF_STMT (arg); >> gcall *call_stmt = dyn_cast<gcall *> (arg_def); >> diff --git a/gcc/testsuite/gcc.dg/ipa/pr83648.c >> b/gcc/testsuite/gcc.dg/ipa/pr83648.c >> new file mode 100644 >> index 00000000000..03b45de671b >> --- /dev/null >> +++ b/gcc/testsuite/gcc.dg/ipa/pr83648.c >> @@ -0,0 +1,9 @@ >> +/* { dg-do compile } */ >> +/* { dg-options "-O2 -fdump-tree-local-pure-const-details" } */ >> + >> +void *g(unsigned n) >> +{ >> + return n ? __builtin_malloc (n) : 0; >> +} >> + >> +/* { dg-final { scan-tree-dump "Function found to be malloc: g" >> "local-pure-const1" } } */ >
diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c index 09ca3590039..dd15a499f6b 100644 --- a/gcc/ipa-pure-const.c +++ b/gcc/ipa-pure-const.c @@ -910,11 +910,13 @@ malloc_candidate_p (function *fun, bool ipa) #define DUMP_AND_RETURN(reason) \ { \ if (dump_file && (dump_flags & TDF_DETAILS)) \ - fprintf (dump_file, "%s", (reason)); \ + fprintf (dump_file, "\n%s is not a malloc candidate, reason: %s\n", \ + (node->name()), (reason)); \ return false; \ } - if (EDGE_COUNT (exit_block->preds) == 0) + if (EDGE_COUNT (exit_block->preds) == 0 + || !flag_delete_null_pointer_checks) return false; FOR_EACH_EDGE (e, ei, exit_block->preds) @@ -958,34 +960,45 @@ malloc_candidate_p (function *fun, bool ipa) } else if (gphi *phi = dyn_cast<gphi *> (def)) - for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i) - { - tree arg = gimple_phi_arg_def (phi, i); - if (TREE_CODE (arg) != SSA_NAME) - DUMP_AND_RETURN("phi arg is not SSA_NAME.") - if (!(arg == null_pointer_node || check_retval_uses (arg, phi))) - DUMP_AND_RETURN("phi arg has uses outside phi" - " and comparisons against 0.") - - gimple *arg_def = SSA_NAME_DEF_STMT (arg); - gcall *call_stmt = dyn_cast<gcall *> (arg_def); - if (!call_stmt) - return false; - tree callee_decl = gimple_call_fndecl (call_stmt); - if (!callee_decl) - return false; - if (!ipa && !DECL_IS_MALLOC (callee_decl)) - DUMP_AND_RETURN("callee_decl does not have malloc attribute for" - " non-ipa mode.") - - cgraph_edge *cs = node->get_edge (call_stmt); - if (cs) - { - ipa_call_summary *es = ipa_call_summaries->get (cs); - gcc_assert (es); - es->is_return_callee_uncaptured = true; - } - } + { + bool nonzero_arg = false; + + for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i) + { + tree arg = gimple_phi_arg_def (phi, i); + if (integer_zerop (arg)) + continue; + + if (TREE_CODE (arg) != SSA_NAME) + DUMP_AND_RETURN ("phi arg is not SSA_NAME."); + if (!check_retval_uses (arg, phi)) + DUMP_AND_RETURN ("phi arg has uses outside phi" + " and comparisons against 0.") + + nonzero_arg = true; + gimple *arg_def = SSA_NAME_DEF_STMT (arg); + gcall *call_stmt = dyn_cast<gcall *> (arg_def); + if (!call_stmt) + return false; + tree callee_decl = gimple_call_fndecl (call_stmt); + if (!callee_decl) + return false; + if (!ipa && !DECL_IS_MALLOC (callee_decl)) + DUMP_AND_RETURN("callee_decl does not have malloc attribute for" + " non-ipa mode.") + + cgraph_edge *cs = node->get_edge (call_stmt); + if (cs) + { + ipa_call_summary *es = ipa_call_summaries->get (cs); + gcc_assert (es); + es->is_return_callee_uncaptured = true; + } + } + + if (!nonzero_arg) + DUMP_AND_RETURN ("all aregs to phi are 0."); + } else DUMP_AND_RETURN("def_stmt of return value is not a call or phi-stmt.") diff --git a/gcc/testsuite/gcc.dg/ipa/pr83648.c b/gcc/testsuite/gcc.dg/ipa/pr83648.c new file mode 100644 index 00000000000..03b45de671b --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr83648.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-local-pure-const-details" } */ + +void *g(unsigned n) +{ + return n ? __builtin_malloc (n) : 0; +} + +/* { dg-final { scan-tree-dump "Function found to be malloc: g" "local-pure-const1" } } */