| Message ID | 1279746011.9821.113.camel@hpsje.cup.hp.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 21, 2010 at 11:00 PM, Steve Ellcey <sje@cup.hp.com> wrote: > > Here is my latest patch to fix this problem. I think the issue involves > the for_return argument to promote_function_mode. The comments for > promote_function_mode simply say that this argument is non-zero if we > are promoting a return value (and not an argument). > > But the routine calls targetm.calls.promote_function_mode and if that > is set to default_promote_function_mode, the code checks for > (for_return == 2) and calls promote_mode if it is. This behaviour > is documented in tm.texi: > > @var{for_return} allows to distinguish the promotion of > arguments and return values. If it is @code{1}, a return value > is being promoted and @code{TARGET_FUNCTION_VALUE} must perform > the same promotions done here. If it is @code{2}, the returned > mode should be that of the register in which an incoming > parameter is copied, or the outgoing result is computed; then > the hook should return the same mode as @code{promote_mode}, > though the signedness may be different. > > Given this, it seems that expand_value_return should check for a > reference return and call promote_function_mode with 2 instead of 1 > for that case. This patch allows me to bootstrap on IA64 and the > testing seems to be going OK. Assuming it works, is this patch OK to > checkin? Hm, it sounds reasonable. But I'd like someone more familiar with the code review the patch. Thanks, Richard. > Steve Ellcey > sje@cup.hp.com > > Index: stmt.c > =================================================================== > --- stmt.c (revision 162360) > +++ stmt.c (working copy) > @@ -1595,8 +1595,11 @@ expand_value_return (rtx val) > tree type = TREE_TYPE (decl); > int unsignedp = TYPE_UNSIGNED (type); > enum machine_mode old_mode = DECL_MODE (decl); > - enum machine_mode mode = promote_function_mode (type, old_mode, > - &unsignedp, funtype, 1); > + enum machine_mode mode; > + if (DECL_BY_REFERENCE (decl)) > + mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 2); > + else > + mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 1); > > if (mode != old_mode) > val = convert_modes (mode, old_mode, val, unsignedp); > >
Richard Guenther <richard.guenther@gmail.com> writes: > On Wed, Jul 21, 2010 at 11:00 PM, Steve Ellcey <sje@cup.hp.com> wrote: >> >> Here is my latest patch to fix this problem. I think the issue involves >> the for_return argument to promote_function_mode. The comments for >> promote_function_mode simply say that this argument is non-zero if we >> are promoting a return value (and not an argument). >> >> But the routine calls targetm.calls.promote_function_mode and if that >> is set to default_promote_function_mode, the code checks for >> (for_return == 2) and calls promote_mode if it is. This behaviour >> is documented in tm.texi: >> >> @var{for_return} allows to distinguish the promotion of >> arguments and return values. If it is @code{1}, a return value >> is being promoted and @code{TARGET_FUNCTION_VALUE} must perform >> the same promotions done here. If it is @code{2}, the returned >> mode should be that of the register in which an incoming >> parameter is copied, or the outgoing result is computed; then >> the hook should return the same mode as @code{promote_mode}, >> though the signedness may be different. >> >> Given this, it seems that expand_value_return should check for a >> reference return and call promote_function_mode with 2 instead of 1 >> for that case. This patch allows me to bootstrap on IA64 and the >> testing seems to be going OK. Assuming it works, is this patch OK to >> checkin? > > Hm, it sounds reasonable. But I'd like someone more familiar with > the code review the patch. This looks OK to me. Thanks. Ian >> =================================================================== >> --- stmt.c (revision 162360) >> +++ stmt.c (working copy) >> @@ -1595,8 +1595,11 @@ expand_value_return (rtx val) >> tree type = TREE_TYPE (decl); >> int unsignedp = TYPE_UNSIGNED (type); >> enum machine_mode old_mode = DECL_MODE (decl); >> - enum machine_mode mode = promote_function_mode (type, old_mode, >> - &unsignedp, funtype, 1); >> + enum machine_mode mode; >> + if (DECL_BY_REFERENCE (decl)) >> + mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 2); >> + else >> + mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 1); >> >> if (mode != old_mode) >> val = convert_modes (mode, old_mode, val, unsignedp); >> >>
On 07/21/2010 02:00 PM, Steve Ellcey wrote: > + enum machine_mode mode; > + if (DECL_BY_REFERENCE (decl)) > + mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 2); > + else > + mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 1); I think the logic is good. I would merely combine the two calls: mode = promote_function_mode (type, old_mode, &unsignedp, funtype, (DECL_BY_REFERENCE (decl) ? 2 : 1)); r~
On Thu, 2010-07-22 at 11:28 -0700, Richard Henderson wrote: > On 07/21/2010 02:00 PM, Steve Ellcey wrote: > > + enum machine_mode mode; > > + if (DECL_BY_REFERENCE (decl)) > > + mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 2); > > + else > > + mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 1); > > I think the logic is good. I would merely combine the two calls: > > mode = promote_function_mode (type, old_mode, &unsignedp, funtype, > (DECL_BY_REFERENCE (decl) ? 2 : 1)); > > > r~ I just checked in the original version a minute ago. Should I change it? Steve Ellcey sje@cup.hp.com
On 07/22/2010 11:31 AM, Steve Ellcey wrote: > I just checked in the original version a minute ago. Should I change > it? I suppose not. Better of course would be to have a pass that performs this optimization automatically of course. ;-) r~
Index: stmt.c =================================================================== --- stmt.c (revision 162360) +++ stmt.c (working copy) @@ -1595,8 +1595,11 @@ expand_value_return (rtx val) tree type = TREE_TYPE (decl); int unsignedp = TYPE_UNSIGNED (type); enum machine_mode old_mode = DECL_MODE (decl); - enum machine_mode mode = promote_function_mode (type, old_mode, - &unsignedp, funtype, 1); + enum machine_mode mode; + if (DECL_BY_REFERENCE (decl)) + mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 2); + else + mode = promote_function_mode (type, old_mode, &unsignedp, funtype, 1); if (mode != old_mode) val = convert_modes (mode, old_mode, val, unsignedp);
Here is my latest patch to fix this problem. I think the issue involves the for_return argument to promote_function_mode. The comments for promote_function_mode simply say that this argument is non-zero if we are promoting a return value (and not an argument). But the routine calls targetm.calls.promote_function_mode and if that is set to default_promote_function_mode, the code checks for (for_return == 2) and calls promote_mode if it is. This behaviour is documented in tm.texi: @var{for_return} allows to distinguish the promotion of arguments and return values. If it is @code{1}, a return value is being promoted and @code{TARGET_FUNCTION_VALUE} must perform the same promotions done here. If it is @code{2}, the returned mode should be that of the register in which an incoming parameter is copied, or the outgoing result is computed; then the hook should return the same mode as @code{promote_mode}, though the signedness may be different. Given this, it seems that expand_value_return should check for a reference return and call promote_function_mode with 2 instead of 1 for that case. This patch allows me to bootstrap on IA64 and the testing seems to be going OK. Assuming it works, is this patch OK to checkin? Steve Ellcey sje@cup.hp.com