[RFC] Return NULL from gimple_call_return_type if no return available.

[RFC] Return NULL from gimple_call_return_type if no return available.

[Aldy Hernandez]

The call to gimple_call_fntype() in gimple_call_return_type() may return
NULL, which causes the TREE_TYPE(lhs) to ICE.  I think it would be best to
return NULL (or void_type_node) rather than aborting.

I'm running into this because fold_using_range::range_of_call, calls
gimple_call_return_type which may ICE for builtins with no LHS.  Instead
of special casing things in range_of_call, perhaps it's best to plug the
source.

Does this sound reasonable?

gcc/ChangeLog:

	* gimple.h (gimple_call_return_type): Return NULL when no type
	and no lhs is available.
---
 gcc/gimple.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gcc/gimple.h b/gcc/gimple.h
index e7dc2a45a13..2a01fe631ec 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -3182,7 +3182,10 @@ gimple_call_return_type (const gcall *gs)
   tree type = gimple_call_fntype (gs);
 
   if (type == NULL_TREE)
-    return TREE_TYPE (gimple_call_lhs (gs));
+    {
+      tree lhs = gimple_call_lhs (gs);
+      return lhs ? TREE_TYPE (lhs) : NULL_TREE;
+    }
 
   /* The type returned by a function is the type of its
      function type.  */
-- 
2.31.1

Re: [RFC] Return NULL from gimple_call_return_type if no return available.

[Richard Biener]

On June 23, 2021 5:03:05 PM GMT+02:00, Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>The call to gimple_call_fntype() in gimple_call_return_type() may
>return
>NULL, which causes the TREE_TYPE(lhs) to ICE.  I think it would be best
>to
>return NULL (or void_type_node) rather than aborting.
>
>I'm running into this because fold_using_range::range_of_call, calls
>gimple_call_return_type which may ICE for builtins with no LHS. 
>Instead
>of special casing things in range_of_call, perhaps it's best to plug
>the
>source.
>
>Does this sound reasonable?

No, you need to make sure to not call this on an internal function call instead. 
Otherwise it is never NULL. 

Richard. 

>gcc/ChangeLog:
>
>	* gimple.h (gimple_call_return_type): Return NULL when no type
>	and no lhs is available.
>---
> gcc/gimple.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/gcc/gimple.h b/gcc/gimple.h
>index e7dc2a45a13..2a01fe631ec 100644
>--- a/gcc/gimple.h
>+++ b/gcc/gimple.h
>@@ -3182,7 +3182,10 @@ gimple_call_return_type (const gcall *gs)
>   tree type = gimple_call_fntype (gs);
> 
>   if (type == NULL_TREE)
>-    return TREE_TYPE (gimple_call_lhs (gs));
>+    {
>+      tree lhs = gimple_call_lhs (gs);
>+      return lhs ? TREE_TYPE (lhs) : NULL_TREE;
>+    }
> 
>   /* The type returned by a function is the type of its
>      function type.  */

Re: [RFC] Return NULL from gimple_call_return_type if no return available.

[Andrew MacLeod]

On 6/23/21 2:37 PM, Richard Biener via Gcc-patches wrote:
> On June 23, 2021 5:03:05 PM GMT+02:00, Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>> The call to gimple_call_fntype() in gimple_call_return_type() may
>> return
>> NULL, which causes the TREE_TYPE(lhs) to ICE.  I think it would be best
>> to
>> return NULL (or void_type_node) rather than aborting.
>>
>> I'm running into this because fold_using_range::range_of_call, calls
>> gimple_call_return_type which may ICE for builtins with no LHS.
>> Instead
>> of special casing things in range_of_call, perhaps it's best to plug
>> the
>> source.
>>
>> Does this sound reasonable?
> No, you need to make sure to not call this on an internal function call instead.
> Otherwise it is never NULL.
>
> Richard.

Out of curiosity, why is it not OK to call this on an internal function 
call?   Shouldn't all calls return something at least? like VOIDmode if 
they don't return anything?  It just seems like it needs to be special 
cased either at any possible call site, or simply in the routine.   we 
stumbled across it and it wasn't obvious why.

Andrew

Re: [RFC] Return NULL from gimple_call_return_type if no return available.

[Richard Biener]

On Wed, Jun 23, 2021 at 9:25 PM Andrew MacLeod <amacleod@redhat.com> wrote:
>
> On 6/23/21 2:37 PM, Richard Biener via Gcc-patches wrote:
> > On June 23, 2021 5:03:05 PM GMT+02:00, Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> >> The call to gimple_call_fntype() in gimple_call_return_type() may
> >> return
> >> NULL, which causes the TREE_TYPE(lhs) to ICE.  I think it would be best
> >> to
> >> return NULL (or void_type_node) rather than aborting.
> >>
> >> I'm running into this because fold_using_range::range_of_call, calls
> >> gimple_call_return_type which may ICE for builtins with no LHS.
> >> Instead
> >> of special casing things in range_of_call, perhaps it's best to plug
> >> the
> >> source.
> >>
> >> Does this sound reasonable?
> > No, you need to make sure to not call this on an internal function call instead.
> > Otherwise it is never NULL.
> >
> > Richard.
>
> Out of curiosity, why is it not OK to call this on an internal function
> call?   Shouldn't all calls return something at least? like VOIDmode if
> they don't return anything?  It just seems like it needs to be special
> cased either at any possible call site, or simply in the routine.   we
> stumbled across it and it wasn't obvious why.

Well, gimple_call_fntype simply returns NULL because internal functions
do not have any API/ABI.  So you either deal with a NULL return value
but then explicitely checking for an internal function call is clearly better
from a source documentation point of view.

I think the existing type == NULL check was likely added to avoid touching
too much code and we somehow didn't think of internal function calls
w/o a LHS (but why are you asking for the return type for a call w/o LHS?)

So yes, you could argue that

  if (type == NULL_TREE)
    {
        gcc_assert (gimple_call_internal_p (gs));
        tree lhs = gimple_call_lhs (gs);
        return lhs ? TREE_TYPE (lhs) : void_type_node;
    }

would make gimple_call_return_type more robust.  But then I wonder
why the function exists in the first place ;)  I suppose like gimple_expr_type
it's one of those I'd eventually see to go away.

Richard.

> Andrew
>
>

Re: [RFC] Return NULL from gimple_call_return_type if no return available.

[Andrew MacLeod]

On 6/24/21 5:07 AM, Richard Biener wrote:
> On Wed, Jun 23, 2021 at 9:25 PM Andrew MacLeod <amacleod@redhat.com> wrote:
>> On 6/23/21 2:37 PM, Richard Biener via Gcc-patches wrote:
>>> On June 23, 2021 5:03:05 PM GMT+02:00, Aldy Hernandez via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>>> The call to gimple_call_fntype() in gimple_call_return_type() may
>>>> return
>>>> NULL, which causes the TREE_TYPE(lhs) to ICE.  I think it would be best
>>>> to
>>>> return NULL (or void_type_node) rather than aborting.
>>>>
>>>> I'm running into this because fold_using_range::range_of_call, calls
>>>> gimple_call_return_type which may ICE for builtins with no LHS.
>>>> Instead
>>>> of special casing things in range_of_call, perhaps it's best to plug
>>>> the
>>>> source.
>>>>
>>>> Does this sound reasonable?
>>> No, you need to make sure to not call this on an internal function call instead.
>>> Otherwise it is never NULL.
>>>
>>> Richard.
>> Out of curiosity, why is it not OK to call this on an internal function
>> call?   Shouldn't all calls return something at least? like VOIDmode if
>> they don't return anything?  It just seems like it needs to be special
>> cased either at any possible call site, or simply in the routine.   we
>> stumbled across it and it wasn't obvious why.
> Well, gimple_call_fntype simply returns NULL because internal functions
> do not have any API/ABI.  So you either deal with a NULL return value
> but then explicitely checking for an internal function call is clearly better
> from a source documentation point of view.
>
> I think the existing type == NULL check was likely added to avoid touching
> too much code and we somehow didn't think of internal function calls
> w/o a LHS (but why are you asking for the return type for a call w/o LHS?)

We'll still compute values for statements that don't have a LHS.. 
there's nothing inherently wrong with that.  The primary example is

if (x_2 < y_3)

we will compute [0,0] [1,1] or [0,1] for that statement, without a LHS.  
It primarily becomes a generic way to ask for the range of each of the 
operands of the statement, and process it regardless of the presence of 
a LHS.  I don't know, maybe there is (or will be)  an internal function 
that doesn't have a LHS but which can be folded away/rewritten if the 
operands are certain values.

Andrew

Re: [RFC] Return NULL from gimple_call_return_type if no return available.

[Jakub Jelinek]

On Thu, Jun 24, 2021 at 09:31:13AM -0400, Andrew MacLeod via Gcc-patches wrote:
> We'll still compute values for statements that don't have a LHS.. there's
> nothing inherently wrong with that.  The primary example is
> 
> if (x_2 < y_3)
> 
> we will compute [0,0] [1,1] or [0,1] for that statement, without a LHS.  It
> primarily becomes a generic way to ask for the range of each of the operands
> of the statement, and process it regardless of the presence of a LHS.  I
> don't know, maybe there is (or will be)  an internal function that doesn't
> have a LHS but which can be folded away/rewritten if the operands are
> certain values.

There are many internal functions that aren't ECF_CONST or ECF_PURE.  Some
of them, like IFN*STORE* I think never have an lhs, others have them, but
if the lhs is unused, various optimization passes can just remove those lhs
from the internal fn calls (if they'd be ECF_CONST or ECF_PURE, the calls
would be DCEd).

I think generally, if a call doesn't have lhs, there is no point in
computing a value range for that missing lhs.  It won't be useful for the
call arguments to lhs direction (nothing would care about that value) and
it won't be useful on the direction from the lhs to the call arguments
either.  Say if one has
  p_23 = __builtin_memcpy (p_75, q_23, 16);
then one can imply from ~[0, 0] range on p_75 that p_23 has that range too
(and vice versa), but if one has
  __builtin_memcpy (p_125, q_23, 16);
none of that makes sense.

So instead of punting when gimple_call_return_type returns NULL IMHO the
code should punt when gimple_call_lhs is NULL.

	Jakub

Re: [RFC] Return NULL from gimple_call_return_type if no return available.

[Andrew MacLeod]

On 6/24/21 9:45 AM, Jakub Jelinek wrote:
> On Thu, Jun 24, 2021 at 09:31:13AM -0400, Andrew MacLeod via Gcc-patches wrote:
>> We'll still compute values for statements that don't have a LHS.. there's
>> nothing inherently wrong with that.  The primary example is
>>
>> if (x_2 < y_3)
>>
>> we will compute [0,0] [1,1] or [0,1] for that statement, without a LHS.  It
>> primarily becomes a generic way to ask for the range of each of the operands
>> of the statement, and process it regardless of the presence of a LHS.  I
>> don't know, maybe there is (or will be)  an internal function that doesn't
>> have a LHS but which can be folded away/rewritten if the operands are
>> certain values.
> There are many internal functions that aren't ECF_CONST or ECF_PURE.  Some
> of them, like IFN*STORE* I think never have an lhs, others have them, but
> if the lhs is unused, various optimization passes can just remove those lhs
> from the internal fn calls (if they'd be ECF_CONST or ECF_PURE, the calls
> would be DCEd).
>
> I think generally, if a call doesn't have lhs, there is no point in
> computing a value range for that missing lhs.  It won't be useful for the
> call arguments to lhs direction (nothing would care about that value) and
> it won't be useful on the direction from the lhs to the call arguments
> either.  Say if one has
>    p_23 = __builtin_memcpy (p_75, q_23, 16);
> then one can imply from ~[0, 0] range on p_75 that p_23 has that range too
> (and vice versa), but if one has
>    __builtin_memcpy (p_125, q_23, 16);
> none of that makes sense.
>
> So instead of punting when gimple_call_return_type returns NULL IMHO the
> code should punt when gimple_call_lhs is NULL.
>
> 	

Well, we are going to punt anyway, because the call type, whether it is 
NULL or VOIDmode is not supported by irange.   It was more just a matter 
of figuring out whether us checking for internal call or the 
gimple_function_return_type call should do the check...   Ultimately in 
the end it doesnt matter.. just seemed like something someone else could 
trip across if we didnt strengthen gimple_call_return_type to not ice.

Andrew

Re: [RFC] Return NULL from gimple_call_return_type if no return available.

[Aldy Hernandez]

[-- Attachment #1: Type: text/plain, Size: 2868 bytes --]

Well, if we don't adjust gimple_call_return_type() to handle built-ins
with no LHS, then we must adjust the callers.

The attached patch fixes gimple_expr_type() per it's documentation:

/* Return the type of the main expression computed by STMT.  Return
   void_type_node if the statement computes nothing.  */

Currently gimple_expr_type is ICEing because it calls gimple_call_return_type.

I still think gimple_call_return_type should return void_type_node
instead of ICEing, but this will also fix my problem.

Anyone have a problem with this?

Aldy

On Thu, Jun 24, 2021 at 3:57 PM Andrew MacLeod via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On 6/24/21 9:45 AM, Jakub Jelinek wrote:
> > On Thu, Jun 24, 2021 at 09:31:13AM -0400, Andrew MacLeod via Gcc-patches wrote:
> >> We'll still compute values for statements that don't have a LHS.. there's
> >> nothing inherently wrong with that.  The primary example is
> >>
> >> if (x_2 < y_3)
> >>
> >> we will compute [0,0] [1,1] or [0,1] for that statement, without a LHS.  It
> >> primarily becomes a generic way to ask for the range of each of the operands
> >> of the statement, and process it regardless of the presence of a LHS.  I
> >> don't know, maybe there is (or will be)  an internal function that doesn't
> >> have a LHS but which can be folded away/rewritten if the operands are
> >> certain values.
> > There are many internal functions that aren't ECF_CONST or ECF_PURE.  Some
> > of them, like IFN*STORE* I think never have an lhs, others have them, but
> > if the lhs is unused, various optimization passes can just remove those lhs
> > from the internal fn calls (if they'd be ECF_CONST or ECF_PURE, the calls
> > would be DCEd).
> >
> > I think generally, if a call doesn't have lhs, there is no point in
> > computing a value range for that missing lhs.  It won't be useful for the
> > call arguments to lhs direction (nothing would care about that value) and
> > it won't be useful on the direction from the lhs to the call arguments
> > either.  Say if one has
> >    p_23 = __builtin_memcpy (p_75, q_23, 16);
> > then one can imply from ~[0, 0] range on p_75 that p_23 has that range too
> > (and vice versa), but if one has
> >    __builtin_memcpy (p_125, q_23, 16);
> > none of that makes sense.
> >
> > So instead of punting when gimple_call_return_type returns NULL IMHO the
> > code should punt when gimple_call_lhs is NULL.
> >
> >
>
> Well, we are going to punt anyway, because the call type, whether it is
> NULL or VOIDmode is not supported by irange.   It was more just a matter
> of figuring out whether us checking for internal call or the
> gimple_function_return_type call should do the check...   Ultimately in
> the end it doesnt matter.. just seemed like something someone else could
> trip across if we didnt strengthen gimple_call_return_type to not ice.
>
> Andrew
>

[-- Attachment #2: curr.patch --]
[-- Type: text/x-patch, Size: 1533 bytes --]

commit 2717e79f571b23f74bb438c27ad1551de8eb9a4d
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Thu Jul 15 12:47:26 2021 +0200

    Handle built-ins with no return TYPE in gimple_expr_type.
    
    Since gimple_call_return_type ICE's, on built-ins with no return types,
    all callers must be adjusted.
    
    gcc/ChangeLog:
    
            * gimple-range-fold.cc (fold_using_range::range_of_call):
            * gimple.h (gimple_expr_type):

diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
index eff5d1f89f2..7d20c6b4b04 100644
--- a/gcc/gimple-range-fold.cc
+++ b/gcc/gimple-range-fold.cc
@@ -780,7 +780,7 @@ fold_using_range::range_of_phi (irange &r, gphi *phi, fur_source &src)
 bool
 fold_using_range::range_of_call (irange &r, gcall *call, fur_source &src)
 {
-  tree type = gimple_call_return_type (call);
+  tree type = gimple_expr_type (call);
   tree lhs = gimple_call_lhs (call);
   bool strict_overflow_p;
 
diff --git a/gcc/gimple.h b/gcc/gimple.h
index acf572b81be..395257eb312 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -6633,6 +6633,13 @@ gimple_expr_type (const gimple *stmt)
 	  default:
 	    break;
 	  }
+
+      // ?? The call to gimple_call_return_type below will ICE on
+      // built-ins with no LHS.  An alternative would be to return
+      // void_type_node from it insteadl.
+      if (!gimple_call_lhs (call_stmt) && gimple_call_internal_p (call_stmt))
+	return void_type_node;
+
       return gimple_call_return_type (call_stmt);
     }
   else if (code == GIMPLE_ASSIGN)

Re: [RFC] Return NULL from gimple_call_return_type if no return available.

[Richard Biener]

On Thu, Jul 15, 2021 at 1:06 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
> Well, if we don't adjust gimple_call_return_type() to handle built-ins
> with no LHS, then we must adjust the callers.
>
> The attached patch fixes gimple_expr_type() per it's documentation:
>
> /* Return the type of the main expression computed by STMT.  Return
>    void_type_node if the statement computes nothing.  */
>
> Currently gimple_expr_type is ICEing because it calls gimple_call_return_type.
>
> I still think gimple_call_return_type should return void_type_node
> instead of ICEing, but this will also fix my problem.
>
> Anyone have a problem with this?

It's still somewhat inconsistent, no?  Because for a call without a LHS
it's now either void_type_node or the type of the return value.

It's probably known I dislike gimple_expr_type itself (it was introduced
to make the transition to tuples easier).  I wonder why you can't simply
fix range_of_call to do

   tree lhs = gimple_call_lhs (call);
   if (lhs)
     type = TREE_TYPE (lhs);

Richard.

>
> Aldy
>
> On Thu, Jun 24, 2021 at 3:57 PM Andrew MacLeod via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > On 6/24/21 9:45 AM, Jakub Jelinek wrote:
> > > On Thu, Jun 24, 2021 at 09:31:13AM -0400, Andrew MacLeod via Gcc-patches wrote:
> > >> We'll still compute values for statements that don't have a LHS.. there's
> > >> nothing inherently wrong with that.  The primary example is
> > >>
> > >> if (x_2 < y_3)
> > >>
> > >> we will compute [0,0] [1,1] or [0,1] for that statement, without a LHS.  It
> > >> primarily becomes a generic way to ask for the range of each of the operands
> > >> of the statement, and process it regardless of the presence of a LHS.  I
> > >> don't know, maybe there is (or will be)  an internal function that doesn't
> > >> have a LHS but which can be folded away/rewritten if the operands are
> > >> certain values.
> > > There are many internal functions that aren't ECF_CONST or ECF_PURE.  Some
> > > of them, like IFN*STORE* I think never have an lhs, others have them, but
> > > if the lhs is unused, various optimization passes can just remove those lhs
> > > from the internal fn calls (if they'd be ECF_CONST or ECF_PURE, the calls
> > > would be DCEd).
> > >
> > > I think generally, if a call doesn't have lhs, there is no point in
> > > computing a value range for that missing lhs.  It won't be useful for the
> > > call arguments to lhs direction (nothing would care about that value) and
> > > it won't be useful on the direction from the lhs to the call arguments
> > > either.  Say if one has
> > >    p_23 = __builtin_memcpy (p_75, q_23, 16);
> > > then one can imply from ~[0, 0] range on p_75 that p_23 has that range too
> > > (and vice versa), but if one has
> > >    __builtin_memcpy (p_125, q_23, 16);
> > > none of that makes sense.
> > >
> > > So instead of punting when gimple_call_return_type returns NULL IMHO the
> > > code should punt when gimple_call_lhs is NULL.
> > >
> > >
> >
> > Well, we are going to punt anyway, because the call type, whether it is
> > NULL or VOIDmode is not supported by irange.   It was more just a matter
> > of figuring out whether us checking for internal call or the
> > gimple_function_return_type call should do the check...   Ultimately in
> > the end it doesnt matter.. just seemed like something someone else could
> > trip across if we didnt strengthen gimple_call_return_type to not ice.
> >
> > Andrew
> >

Re: [RFC] Return NULL from gimple_call_return_type if no return available.

[Aldy Hernandez]

On 7/15/21 3:06 PM, Richard Biener wrote:
> On Thu, Jul 15, 2021 at 1:06 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>> Well, if we don't adjust gimple_call_return_type() to handle built-ins
>> with no LHS, then we must adjust the callers.
>>
>> The attached patch fixes gimple_expr_type() per it's documentation:
>>
>> /* Return the type of the main expression computed by STMT.  Return
>>     void_type_node if the statement computes nothing.  */
>>
>> Currently gimple_expr_type is ICEing because it calls gimple_call_return_type.
>>
>> I still think gimple_call_return_type should return void_type_node
>> instead of ICEing, but this will also fix my problem.
>>
>> Anyone have a problem with this?
> 
> It's still somewhat inconsistent, no?  Because for a call without a LHS
> it's now either void_type_node or the type of the return value.
> 
> It's probably known I dislike gimple_expr_type itself (it was introduced
> to make the transition to tuples easier).  I wonder why you can't simply
> fix range_of_call to do
> 
>     tree lhs = gimple_call_lhs (call);
>     if (lhs)
>       type = TREE_TYPE (lhs);

That would still leave gimple_expr_type() broken.  It's comment clearly 
says it should return void_type_node.

I still think we should just fix gimple_call_return_type to return 
void_type_node instead of ICEing.

Re: [RFC] Return NULL from gimple_call_return_type if no return available.

[Richard Biener]

On Thu, Jul 15, 2021 at 3:16 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 7/15/21 3:06 PM, Richard Biener wrote:
> > On Thu, Jul 15, 2021 at 1:06 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>
> >> Well, if we don't adjust gimple_call_return_type() to handle built-ins
> >> with no LHS, then we must adjust the callers.
> >>
> >> The attached patch fixes gimple_expr_type() per it's documentation:
> >>
> >> /* Return the type of the main expression computed by STMT.  Return
> >>     void_type_node if the statement computes nothing.  */
> >>
> >> Currently gimple_expr_type is ICEing because it calls gimple_call_return_type.
> >>
> >> I still think gimple_call_return_type should return void_type_node
> >> instead of ICEing, but this will also fix my problem.
> >>
> >> Anyone have a problem with this?
> >
> > It's still somewhat inconsistent, no?  Because for a call without a LHS
> > it's now either void_type_node or the type of the return value.
> >
> > It's probably known I dislike gimple_expr_type itself (it was introduced
> > to make the transition to tuples easier).  I wonder why you can't simply
> > fix range_of_call to do
> >
> >     tree lhs = gimple_call_lhs (call);
> >     if (lhs)
> >       type = TREE_TYPE (lhs);
>
> That would still leave gimple_expr_type() broken.  It's comment clearly
> says it should return void_type_node.

Does it?  What does it say for

int foo ();

and the stmt

 'foo ();'

?  How's this different from

 'bar ();'

when bar is an internal function?  Note how the comment
speaks about 'type of the main EXPRESSION' and
'if the STATEMEMT computes nothing' (emphasis mine).
I don't think it's all that clear.  A gimple_cond stmt
doesn't compute anything, does it?  Does the 'foo ()'
statement compute anything?  The current implementation
(and your patched one) says so.  But why does

 .ADD_OVERFLOW (_1, _2);

not (according to your patched implementation)?  It computes
something and that something has a type that depends on
the types of _1 and _2 and on the actual internal function.
But we don't have it readily available.  If you need it then
you are on your own - but returning void_type_node is wrong.

Richard.

> I still think we should just fix gimple_call_return_type to return
> void_type_node instead of ICEing.
>

Re: [RFC] Return NULL from gimple_call_return_type if no return available.

[Richard Biener]

On Thu, Jul 15, 2021 at 3:21 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, Jul 15, 2021 at 3:16 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >
> >
> >
> > On 7/15/21 3:06 PM, Richard Biener wrote:
> > > On Thu, Jul 15, 2021 at 1:06 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> > >>
> > >> Well, if we don't adjust gimple_call_return_type() to handle built-ins
> > >> with no LHS, then we must adjust the callers.
> > >>
> > >> The attached patch fixes gimple_expr_type() per it's documentation:
> > >>
> > >> /* Return the type of the main expression computed by STMT.  Return
> > >>     void_type_node if the statement computes nothing.  */
> > >>
> > >> Currently gimple_expr_type is ICEing because it calls gimple_call_return_type.
> > >>
> > >> I still think gimple_call_return_type should return void_type_node
> > >> instead of ICEing, but this will also fix my problem.
> > >>
> > >> Anyone have a problem with this?
> > >
> > > It's still somewhat inconsistent, no?  Because for a call without a LHS
> > > it's now either void_type_node or the type of the return value.
> > >
> > > It's probably known I dislike gimple_expr_type itself (it was introduced
> > > to make the transition to tuples easier).  I wonder why you can't simply
> > > fix range_of_call to do
> > >
> > >     tree lhs = gimple_call_lhs (call);
> > >     if (lhs)
> > >       type = TREE_TYPE (lhs);
> >
> > That would still leave gimple_expr_type() broken.  It's comment clearly
> > says it should return void_type_node.
>
> Does it?  What does it say for
>
> int foo ();
>
> and the stmt
>
>  'foo ();'
>
> ?  How's this different from
>
>  'bar ();'
>
> when bar is an internal function?  Note how the comment
> speaks about 'type of the main EXPRESSION' and
> 'if the STATEMEMT computes nothing' (emphasis mine).
> I don't think it's all that clear.  A gimple_cond stmt
> doesn't compute anything, does it?  Does the 'foo ()'
> statement compute anything?  The current implementation
> (and your patched one) says so.  But why does
>
>  .ADD_OVERFLOW (_1, _2);
>
> not (according to your patched implementation)?  It computes
> something and that something has a type that depends on
> the types of _1 and _2 and on the actual internal function.
> But we don't have it readily available.  If you need it then
> you are on your own - but returning void_type_node is wrong.

That said, in 99% of all cases people should have used
TREE_TYPE (gimple_get_lhs (stmt)) insead of
gimple_expr_type since that makes clear that we're
talking of a result that materializes somewhere.  It also
makes the required guard obvious - gimple_get_lhs (stmt) != NULL.

Then there are the legacy callers that call it on a GIMPLE_COND
and the (IMHO broken) ones that expect it to do magic for
masked loads and stores.

Richard.

> Richard.
>
> > I still think we should just fix gimple_call_return_type to return
> > void_type_node instead of ICEing.
> >

Re: [RFC] Return NULL from gimple_call_return_type if no return available.

[Richard Biener]

On Thu, Jul 15, 2021 at 3:23 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, Jul 15, 2021 at 3:21 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Thu, Jul 15, 2021 at 3:16 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> > >
> > >
> > >
> > > On 7/15/21 3:06 PM, Richard Biener wrote:
> > > > On Thu, Jul 15, 2021 at 1:06 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> > > >>
> > > >> Well, if we don't adjust gimple_call_return_type() to handle built-ins
> > > >> with no LHS, then we must adjust the callers.
> > > >>
> > > >> The attached patch fixes gimple_expr_type() per it's documentation:
> > > >>
> > > >> /* Return the type of the main expression computed by STMT.  Return
> > > >>     void_type_node if the statement computes nothing.  */
> > > >>
> > > >> Currently gimple_expr_type is ICEing because it calls gimple_call_return_type.
> > > >>
> > > >> I still think gimple_call_return_type should return void_type_node
> > > >> instead of ICEing, but this will also fix my problem.
> > > >>
> > > >> Anyone have a problem with this?
> > > >
> > > > It's still somewhat inconsistent, no?  Because for a call without a LHS
> > > > it's now either void_type_node or the type of the return value.
> > > >
> > > > It's probably known I dislike gimple_expr_type itself (it was introduced
> > > > to make the transition to tuples easier).  I wonder why you can't simply
> > > > fix range_of_call to do
> > > >
> > > >     tree lhs = gimple_call_lhs (call);
> > > >     if (lhs)
> > > >       type = TREE_TYPE (lhs);
> > >
> > > That would still leave gimple_expr_type() broken.  It's comment clearly
> > > says it should return void_type_node.
> >
> > Does it?  What does it say for
> >
> > int foo ();
> >
> > and the stmt
> >
> >  'foo ();'
> >
> > ?  How's this different from
> >
> >  'bar ();'
> >
> > when bar is an internal function?  Note how the comment
> > speaks about 'type of the main EXPRESSION' and
> > 'if the STATEMEMT computes nothing' (emphasis mine).
> > I don't think it's all that clear.  A gimple_cond stmt
> > doesn't compute anything, does it?  Does the 'foo ()'
> > statement compute anything?  The current implementation
> > (and your patched one) says so.  But why does
> >
> >  .ADD_OVERFLOW (_1, _2);
> >
> > not (according to your patched implementation)?  It computes
> > something and that something has a type that depends on
> > the types of _1 and _2 and on the actual internal function.
> > But we don't have it readily available.  If you need it then
> > you are on your own - but returning void_type_node is wrong.
>
> That said, in 99% of all cases people should have used
> TREE_TYPE (gimple_get_lhs (stmt)) insead of
> gimple_expr_type since that makes clear that we're
> talking of a result that materializes somewhere.  It also
> makes the required guard obvious - gimple_get_lhs (stmt) != NULL.
>
> Then there are the legacy callers that call it on a GIMPLE_COND
> and the (IMHO broken) ones that expect it to do magic for
> masked loads and stores.

Btw, void_type_node is also wrong for a GIMPLE_ASM with outputs.

I think if you really want to fix the ICEing then return NULL for
"we don't know" and adjust the current default as well.

Richard.

> Richard.
>
> > Richard.
> >
> > > I still think we should just fix gimple_call_return_type to return
> > > void_type_node instead of ICEing.
> > >

[COMMITTED] Add gimple_range_type for statements.

[Andrew MacLeod]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

On 7/15/21 9:06 AM, Richard Biener wrote:
> On Thu, Jul 15, 2021 at 1:06 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>> Currently gimple_expr_type is ICEing because it calls gimple_call_return_type.
>>
>> I still think gimple_call_return_type should return void_type_node
>> instead of ICEing, but this will also fix my problem.
>>
>> Anyone have a problem with this?
> It's still somewhat inconsistent, no?  Because for a call without a LHS
> it's now either void_type_node or the type of the return value.
>
> It's probably known I dislike gimple_expr_type itself (it was introduced
> to make the transition to tuples easier).  I wonder why you can't simply
> fix range_of_call to do
>
>     tree lhs = gimple_call_lhs (call);
>     if (lhs)
>       type = TREE_TYPE (lhs);
>
> Richard.

You are correct. There are indeed inconsistencies, and they exist in 
multiple places.  In fact, none of them do exactly what we are looking 
for all the time, and there are times we do care about the stmt when 
there is no LHS.    In addition, we almost always then have to check 
whether the type we found is supported.

So instead, much as we did for types with range_compatible_p (), we'll 
provide a function for statements which does exactly what we need. This 
patch eliminates all the ranger calls to both gimple_expr_type ()  and  
gimple_call_return_type () .    This will also simplify the life of 
anyone who goes to eventually remove gimple_expr_type () as there will 
now be less uses.

The function will return a type if and only if we can find the type in 
an orderly fashion, and then determine if it is also supported by ranger.

Bootstrapped on x86_64-pc-linux-gnu with no regressions.  Pushed.

Andrew


[-- Attachment #2: type.patch --]
[-- Type: text/x-patch, Size: 6446 bytes --]

commit 478cc962ad174bfc64c573152a0658935651fce3
Author: Andrew MacLeod <amacleod@redhat.com>
Date:   Thu Jul 15 11:07:12 2021 -0400

    Add gimple_range_type for statements.
    
    The existing mechanisms for picking up the type of a statement are
    inconsistent with the needs of ranger. Encapsulate all the bits
    required to pick up the return type of a statement in one place, and check
    whether the type is supported.
    
            * gimple-range-fold.cc (adjust_pointer_diff_expr): Use
            gimple_range_type.
            (fold_using_range::fold_stmt): Ditto.
            (fold_using_range::range_of_range_op): Ditto.
            (fold_using_range::range_of_phi): Ditto.
            (fold_using_range::range_of_call): Ditto.
            (fold_using_range::range_of_builtin_ubsan_call): Ditto.
            (fold_using_range::range_of_builtin_call): Ditto.
            (fold_using_range::range_of_cond_expr): Ditto.
            * gimple-range-fold.h (gimple_range_type): New.

diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
index eff5d1f89f2..f8578c013bc 100644
--- a/gcc/gimple-range-fold.cc
+++ b/gcc/gimple-range-fold.cc
@@ -362,7 +362,7 @@ adjust_pointer_diff_expr (irange &res, const gimple *diff_stmt)
     {
       tree max = vrp_val_max (ptrdiff_type_node);
       wide_int wmax = wi::to_wide (max, TYPE_PRECISION (TREE_TYPE (max)));
-      tree expr_type = gimple_expr_type (diff_stmt);
+      tree expr_type = gimple_range_type (diff_stmt);
       tree range_min = build_zero_cst (expr_type);
       tree range_max = wide_int_to_tree (expr_type, wmax - 1);
       int_range<2> r (range_min, range_max);
@@ -522,16 +522,8 @@ fold_using_range::fold_stmt (irange &r, gimple *s, fur_source &src, tree name)
 
   if (!res)
     {
-      // If no name is specified, try the expression kind.
-      if (!name)
-	{
-	  tree t = gimple_expr_type (s);
-	  if (!irange::supports_type_p (t))
-	    return false;
-	  r.set_varying (t);
-	  return true;
-	}
-      if (!gimple_range_ssa_p (name))
+      // If no name specified or range is unsupported, bail.
+      if (!name || !gimple_range_ssa_p (name))
 	return false;
       // We don't understand the stmt, so return the global range.
       r = gimple_range_global (name);
@@ -558,10 +550,11 @@ bool
 fold_using_range::range_of_range_op (irange &r, gimple *s, fur_source &src)
 {
   int_range_max range1, range2;
-  tree type = gimple_expr_type (s);
+  tree type = gimple_range_type (s);
+  if (!type)
+    return false;
   range_operator *handler = gimple_range_handler (s);
   gcc_checking_assert (handler);
-  gcc_checking_assert (irange::supports_type_p (type));
 
   tree lhs = gimple_get_lhs (s);
   tree op1 = gimple_range_operand1 (s);
@@ -719,11 +712,11 @@ bool
 fold_using_range::range_of_phi (irange &r, gphi *phi, fur_source &src)
 {
   tree phi_def = gimple_phi_result (phi);
-  tree type = TREE_TYPE (phi_def);
+  tree type = gimple_range_type (phi);
   int_range_max arg_range;
   unsigned x;
 
-  if (!irange::supports_type_p (type))
+  if (!type)
     return false;
 
   // Start with an empty range, unioning in each argument's range.
@@ -780,13 +773,13 @@ fold_using_range::range_of_phi (irange &r, gphi *phi, fur_source &src)
 bool
 fold_using_range::range_of_call (irange &r, gcall *call, fur_source &src)
 {
-  tree type = gimple_call_return_type (call);
+  tree type = gimple_range_type (call);
+  if (!type)
+    return false;
+
   tree lhs = gimple_call_lhs (call);
   bool strict_overflow_p;
 
-  if (!irange::supports_type_p (type))
-    return false;
-
   if (range_of_builtin_call (r, call, src))
     ;
   else if (gimple_stmt_nonnegative_warnv_p (call, &strict_overflow_p))
@@ -817,7 +810,7 @@ fold_using_range::range_of_builtin_ubsan_call (irange &r, gcall *call,
 {
   gcc_checking_assert (code == PLUS_EXPR || code == MINUS_EXPR
 		       || code == MULT_EXPR);
-  tree type = gimple_call_return_type (call);
+  tree type = gimple_range_type (call);
   range_operator *op = range_op_handler (code, type);
   gcc_checking_assert (op);
   int_range_max ir0, ir1;
@@ -853,7 +846,7 @@ fold_using_range::range_of_builtin_call (irange &r, gcall *call,
   if (func == CFN_LAST)
     return false;
 
-  tree type = gimple_call_return_type (call);
+  tree type = gimple_range_type (call);
   tree arg;
   int mini, maxi, zerov = 0, prec;
   scalar_int_mode mode;
@@ -1094,12 +1087,12 @@ fold_using_range::range_of_cond_expr  (irange &r, gassign *s, fur_source &src)
   tree op1 = gimple_assign_rhs2 (s);
   tree op2 = gimple_assign_rhs3 (s);
 
-  gcc_checking_assert (gimple_assign_rhs_code (s) == COND_EXPR);
-  gcc_checking_assert (useless_type_conversion_p  (TREE_TYPE (op1),
-						   TREE_TYPE (op2)));
-  if (!irange::supports_type_p (TREE_TYPE (op1)))
+  tree type = gimple_range_type (s);
+  if (!type)
     return false;
 
+  gcc_checking_assert (gimple_assign_rhs_code (s) == COND_EXPR);
+  gcc_checking_assert (range_compatible_p (TREE_TYPE (op1), TREE_TYPE (op2)));
   src.get_operand (cond_range, cond);
   src.get_operand (range1, op1);
   src.get_operand (range2, op2);
@@ -1118,6 +1111,7 @@ fold_using_range::range_of_cond_expr  (irange &r, gassign *s, fur_source &src)
       r = range1;
       r.union_ (range2);
     }
+  gcc_checking_assert (range_compatible_p (r.type (), type));
   return true;
 }
 
diff --git a/gcc/gimple-range-fold.h b/gcc/gimple-range-fold.h
index dc1b28f9acc..ceed7ba48e1 100644
--- a/gcc/gimple-range-fold.h
+++ b/gcc/gimple-range-fold.h
@@ -56,6 +56,36 @@ gimple_range_handler (const gimple *s)
   return NULL;
 }
 
+// Return the type of range which statement S calculates.  If the type is
+// unsupported or no type can be determined, return NULL_TREE.
+
+static inline tree
+gimple_range_type (const gimple *s)
+{
+  tree lhs = gimple_get_lhs (s);
+  tree type = NULL_TREE;
+  if (lhs)
+    type = TREE_TYPE (lhs);
+  else
+    {
+      enum gimple_code code = gimple_code (s);
+      if (code == GIMPLE_COND)
+	type = boolean_type_node;
+      else if (code == GIMPLE_PHI)
+	type = TREE_TYPE (gimple_phi_result (s));
+      else if (code == GIMPLE_CALL)
+	{
+	  type = gimple_call_fntype (s);
+	  // If it has a type, get the return type.
+	  if (type)
+	    type = TREE_TYPE (type);
+	}
+    }
+  if (irange::supports_type_p (type))
+    return type;
+  return NULL_TREE;
+}
+
 // Return EXP if it is an SSA_NAME with a type supported by gimple ranges.
 
 static inline tree

Re: [COMMITTED] Add gimple_range_type for statements.

[Richard Biener]

On Thu, Jul 15, 2021 at 10:00 PM Andrew MacLeod <amacleod@redhat.com> wrote:
>
> On 7/15/21 9:06 AM, Richard Biener wrote:
> > On Thu, Jul 15, 2021 at 1:06 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >>
> >> Currently gimple_expr_type is ICEing because it calls gimple_call_return_type.
> >>
> >> I still think gimple_call_return_type should return void_type_node
> >> instead of ICEing, but this will also fix my problem.
> >>
> >> Anyone have a problem with this?
> > It's still somewhat inconsistent, no?  Because for a call without a LHS
> > it's now either void_type_node or the type of the return value.
> >
> > It's probably known I dislike gimple_expr_type itself (it was introduced
> > to make the transition to tuples easier).  I wonder why you can't simply
> > fix range_of_call to do
> >
> >     tree lhs = gimple_call_lhs (call);
> >     if (lhs)
> >       type = TREE_TYPE (lhs);
> >
> > Richard.
>
> You are correct. There are indeed inconsistencies, and they exist in
> multiple places.  In fact, none of them do exactly what we are looking
> for all the time, and there are times we do care about the stmt when
> there is no LHS.    In addition, we almost always then have to check
> whether the type we found is supported.
>
> So instead, much as we did for types with range_compatible_p (), we'll
> provide a function for statements which does exactly what we need. This
> patch eliminates all the ranger calls to both gimple_expr_type ()  and
> gimple_call_return_type () .    This will also simplify the life of
> anyone who goes to eventually remove gimple_expr_type () as there will
> now be less uses.

Thanks a lot!

Richard.

> The function will return a type if and only if we can find the type in
> an orderly fashion, and then determine if it is also supported by ranger.
>
> Bootstrapped on x86_64-pc-linux-gnu with no regressions.  Pushed.
>
> Andrew
>