On 12 January 2018 at 05:02, Jeff Law <l...@redhat.com> wrote:
> On 01/10/2018 10:04 PM, Prathamesh Kulkarni wrote:
>> On 11 January 2018 at 04:50, Jeff Law <l...@redhat.com> wrote:
>>> On 01/09/2018 05:57 AM, Prathamesh Kulkarni wrote:
>>>>
>>>> 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 ?
>>> This seems like a pretty significant concern. Given:
>>>
>>>
>>> return n ? 0 : __builtin_malloc (n);
>>>
>>> Is the function malloc-like enough to allow it to be marked?
>>>
>>> If not, then ISTM we have to be very conservative in what we mark.
>>>
>>> foo (n, m)
>>> {
>>> return n ? 0 : __builtin_malloc (m);
>>> }
>>>
>>> Is that malloc-like enough to mark?
>> Not sure. Should I make it more conservative by marking it as malloc
>> only if the argument to __builtin_malloc
>> is constant or it's value-range is known not to include 0? And
>> similarly for __builtin_calloc ?
> It looks like the consensus is we don't need to worry about the cases
> above. So unless Jakub chimes in with a solid reason, don't worry about
> them.
Thanks everyone for the clarification. The attached patch skips on 0 phi arg,
and returns false if -fno-delete-null-pointer-checks is passed.
With the patch, malloc_candidate_p returns true for
return 0;
or
ret = phi<0, 0>
return ret
which I believe is OK as far as correctness is concerned.
However as Martin points out suggesting malloc attribute for return 0
case is not ideal.
I suppose we can track the return 0 (or when value range of return
value is known not to include 0)
corner case and avoid suggesting malloc for those ?
Validation in progress.
Is this patch OK for next stage-1 ?
Thanks,
Prathamesh
>
> Jeff
diff --git a/gcc/ipa-pure-const.c b/gcc/ipa-pure-const.c
index 09ca3590039..3cf71d4c653 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)
@@ -929,6 +931,9 @@ malloc_candidate_p (function *fun, bool ipa)
if (!retval)
DUMP_AND_RETURN("No return value.")
+ if (integer_zerop (retval))
+ continue;
+
if (TREE_CODE (retval) != SSA_NAME
|| TREE_CODE (TREE_TYPE (retval)) != POINTER_TYPE)
DUMP_AND_RETURN("Return value is not SSA_NAME or not a pointer type.")
@@ -961,11 +966,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 (integer_zerop (arg))
+ continue;
+
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.")
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-2.c
b/gcc/testsuite/gcc.dg/ipa/pr83648-2.c
new file mode 100644
index 00000000000..ffa7e462abe
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr83648-2.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-delete-null-pointer-checks
-fdump-tree-local-pure-const-details" } */
+
+void *g(unsigned n)
+{
+ return n ? __builtin_malloc (n) : 0;
+}
+
+void *h()
+{
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump-not "Function found to be malloc: g"
"local-pure-const1" } } */
+/* { dg-final { scan-tree-dump-not "Function found to be malloc: h"
"local-pure-const1" } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/pr83648.c
b/gcc/testsuite/gcc.dg/ipa/pr83648.c
new file mode 100644
index 00000000000..febfd7d9319
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr83648.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-local-pure-const-details" } */
+
+void *g(unsigned n)
+{
+ return n ? __builtin_malloc (n) : 0;
+}
+
+void *h()
+{
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump "Function found to be malloc: g"
"local-pure-const1" } } */
+/* { dg-final { scan-tree-dump "Function found to be malloc: h"
"local-pure-const1" } } */
