diff mbox series

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

Message ID 20210623150305.411460-1-aldyh@redhat.com
State New
Headers show
Series [RFC] Return NULL from gimple_call_return_type if no return available. | expand

Commit Message

Aldy Hernandez June 23, 2021, 3:03 p.m. UTC 
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(-)

Comments

Richard Biener June 23, 2021, 6:37 p.m. UTC | #1 
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.  */
Andrew MacLeod June 23, 2021, 7:25 p.m. UTC | #2 
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
Richard Biener June 24, 2021, 9:07 a.m. UTC | #3 
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
>
>
Andrew MacLeod June 24, 2021, 1:31 p.m. UTC | #4 
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
Jakub Jelinek June 24, 2021, 1:45 p.m. UTC | #5 
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
Andrew MacLeod June 24, 2021, 1:55 p.m. UTC | #6 
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
Aldy Hernandez July 15, 2021, 11:05 a.m. UTC | #7 
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
>
Richard Biener July 15, 2021, 1:06 p.m. UTC | #8 
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
> >
Aldy Hernandez July 15, 2021, 1:16 p.m. UTC | #9 
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.
Richard Biener July 15, 2021, 1:21 p.m. UTC | #10 
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.
>
Richard Biener July 15, 2021, 1:23 p.m. UTC | #11 
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.
> >
Richard Biener July 15, 2021, 1:26 p.m. UTC | #12 
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.
> > >
diff mbox series

Patch

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.  */