| Message ID | BANLkTikorR2hgw1_7eMLMn6bgaPLW4ZW6w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Ping. On Sat, Jun 25, 2011 at 9:06 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Thu, Jun 16, 2011 at 10:18 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Thu, Jun 16, 2011 at 12:56 AM, Richard Guenther >> <richard.guenther@gmail.com> wrote: >>> On Wed, Jun 15, 2011 at 9:55 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>> On Wed, Jun 15, 2011 at 8:16 AM, Michael Matz <matz@suse.de> wrote: >>>>> Hi, >>>>> >>>>> On Wed, 15 Jun 2011, H.J. Lu wrote: >>>>> >>>>>> >> + /* FIXME: update_nonlocal_goto_save_area may pass SA in the wrong mode. */ >>>>>> >> + if (GET_MODE (sa) != mode) >>>>>> >> + { >>>>>> >> + gcc_assert (ptr_mode != Pmode >>>>>> >> + && GET_MODE (sa) == ptr_mode >>>>>> >> + && mode == Pmode); >>>>>> >> + sa = adjust_address (sa, mode, 0); >>>>>> >> + } >>>>>> > >>>>>> > That may be appropriate for a branch, but trunk shouldn't contain FIXMEs >>>>>> > that explain how something should be fixed, instead that something should >>>>>> > be carried out. I.e. just fix update_nonlocal_goto_save_area. >>>>>> > >>>>>> >>>>>> I don't know update_nonlocal_goto_save_area enough to fix it >>>>>> without breaking other targets. This patch is the lest invasive. >>>>>> Any suggestions how to properly fix it is appreciated. >>>>> >>>>> Well, the most obvious variant would be to move the above code right >>>>> before the call of emit_stack_save in update_nonlocal_goto_save_area >>>>> (using r_save and STACK_SAVEAREA_MODE (SAVE_NONLOCAL)). All other callers >>>>> of emit_stack_save already make sure to pass an object of correct mode, so >>>>> this one should too. >>>>> >>>>> But I think it's better to just produce a correct array_ref from the >>>>> start. get_nl_goto_field creates an array_type for the >>>>> nonlocal_goto_save_area of correct type (ptr_type_node or >>>>> lang_hooks.types.type_for_mode (Pmode, 1)), and we should use that. >>>>> >>>>> So something like this in update_nonlocal_goto_save_area: >>>>> t_save = build4 (ARRAY_REF, >>>>> TREE_TYPE (TREE_TYPE (cfun->nonlocal_goto_save_area)), >>>>> cfun->nonlocal_goto_save_area, >>>>> integer_one_node, NULL_TREE, NULL_TREE); >>>>> >>>>> instead of the current building of t_save. Then r_save also should get >>>>> the correct mode automatically. >>>>> >>>> >>>> Here is the updated patch. OK for trunk? >>> >>> The explow.c change is ok. For the function.c change I wonder why >>> convert_memory_address doesn't do the right thing - from it's documentation >>> it definitely should, so it should be fixed instead of being replaced by >>> adjust_address with a zero offset. >>> >> >> convert_memory_address may return a pseudo register converted >> to Pmode. But here what we want is the same memory address >> adjusted for Pmode. I don't think the usage of convert_memory_address >> > > Here is the code in question: > > r_save = convert_memory_address (Pmode, r_save); > > emit_move_insn (r_save, targetm.builtin_setjmp_frame_value ()); > > R_SAVE must be lvalue. But return from convert_memory_address > isn't. I am re-posting my patch here. OK for trunk? > > Thanks. > > -- > H.J. > --- > 2011-06-15 H.J. Lu <hongjiu.lu@intel.com> > > PR middle-end/48016 > * explow.c (update_nonlocal_goto_save_area): Use proper mode > for stack save area. > > * function.c (expand_function_start): Properly store frame > pointer for non-local goto. > > diff --git a/gcc/explow.c b/gcc/explow.c > index c7d8183..efe6c7e 100644 > --- a/gcc/explow.c > +++ b/gcc/explow.c > @@ -1102,7 +1097,9 @@ update_nonlocal_goto_save_area (void) > first one is used for the frame pointer save; the rest are sized by > STACK_SAVEAREA_MODE. Create a reference to array index 1, the first > of the stack save area slots. */ > - t_save = build4 (ARRAY_REF, ptr_type_node, cfun->nonlocal_goto_save_area, > + t_save = build4 (ARRAY_REF, > + TREE_TYPE (TREE_TYPE (cfun->nonlocal_goto_save_area)), > + cfun->nonlocal_goto_save_area, > integer_one_node, NULL_TREE, NULL_TREE); > r_save = expand_expr (t_save, NULL_RTX, VOIDmode, EXPAND_WRITE); > > diff --git a/gcc/function.c b/gcc/function.c > index 81c4d39..131bc09 100644 > --- a/gcc/function.c > +++ b/gcc/function.c > @@ -4780,7 +4780,7 @@ expand_function_start (tree subr) > cfun->nonlocal_goto_save_area, > integer_zero_node, NULL_TREE, NULL_TREE); > r_save = expand_expr (t_save, NULL_RTX, VOIDmode, EXPAND_WRITE); > - r_save = convert_memory_address (Pmode, r_save); > + r_save = adjust_address (r_save, Pmode, 0); > > emit_move_insn (r_save, targetm.builtin_setjmp_frame_value ()); > update_nonlocal_goto_save_area (); >
Hi, On Wed, 29 Jun 2011, H.J. Lu wrote: > > diff --git a/gcc/function.c b/gcc/function.c > > index 81c4d39..131bc09 100644 > > --- a/gcc/function.c > > +++ b/gcc/function.c > > @@ -4780,7 +4780,7 @@ expand_function_start (tree subr) > > cfun->nonlocal_goto_save_area, > > integer_zero_node, NULL_TREE, NULL_TREE); > > r_save = expand_expr (t_save, NULL_RTX, VOIDmode, EXPAND_WRITE); > > - r_save = convert_memory_address (Pmode, r_save); > > + r_save = adjust_address (r_save, Pmode, 0); This is actually the same problem as in explow.c. t_save is built with ptr_type_node, where it should have been using TREE_TYPE (TREE_TYPE (cfun->nonlocal_goto_save_area)) Then r_save should have the correct mode already, possibly this could be asserted. You are right that r_save needs to correspond to the nonlocal_goto_save_area[0] array-ref, hence pseudos aren't okay, therefore convert_memory_address isn't. Actually I think we might even want to assert that indeed the expanded r_save is of Pmode already. Ciao, Michael.
diff --git a/gcc/explow.c b/gcc/explow.c index c7d8183..efe6c7e 100644 --- a/gcc/explow.c +++ b/gcc/explow.c @@ -1102,7 +1097,9 @@ update_nonlocal_goto_save_area (void) first one is used for the frame pointer save; the rest are sized by STACK_SAVEAREA_MODE. Create a reference to array index 1, the first of the stack save area slots. */ - t_save = build4 (ARRAY_REF, ptr_type_node, cfun->nonlocal_goto_save_area, + t_save = build4 (ARRAY_REF, + TREE_TYPE (TREE_TYPE (cfun->nonlocal_goto_save_area)), + cfun->nonlocal_goto_save_area, integer_one_node, NULL_TREE, NULL_TREE); r_save = expand_expr (t_save, NULL_RTX, VOIDmode, EXPAND_WRITE); diff --git a/gcc/function.c b/gcc/function.c index 81c4d39..131bc09 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -4780,7 +4780,7 @@ expand_function_start (tree subr) cfun->nonlocal_goto_save_area, integer_zero_node, NULL_TREE, NULL_TREE); r_save = expand_expr (t_save, NULL_RTX, VOIDmode, EXPAND_WRITE); - r_save = convert_memory_address (Pmode, r_save); + r_save = adjust_address (r_save, Pmode, 0); emit_move_insn (r_save, targetm.builtin_setjmp_frame_value ()); update_nonlocal_goto_save_area ();