| Message ID | BANLkTi=WRPPqXrFvwMsGhd7wghyVi8w=QA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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. Richard. > 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. >
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
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 ();