RE: [PATCH 2/7]: Ping3: Merge from Stack Branch - collect alignment info

["Ye, Joey" <joey dot ye at intel dot com>]

Here is the updated patch for reload1.c. A correctness to previous
email: we added serveral assertions and changed frame pointer handling
for stack alignment.

Thanks - Joey

-----Original Message-----
From: Ye, Joey 
Sent: Wednesday, May 28, 2008 5:29 PM
To: 'Richard Guenther'; Mark Mitchell
Cc: gcc-patches@gcc.gnu.org; Lu, Hongjiu; Guo, Xuepeng; Jan Hubicka;
jason@redhat.com; Ian Lance Taylor
Subject: RE: [PATCH 2/7]: Ping3: Merge from Stack Branch - collect
alignment info

Rich, Thanks for quick response. All three comments are reasonable to
me. I'll post a new version soon.

Mark, The reload is only inserted with several assertions. Would do
please take a look at it?

- Joey

-----Original Message-----
From: Richard Guenther [mailto:richard.guenther@gmail.com] 
Sent: Wednesday, May 28, 2008 5:19 PM
To: Ye, Joey
Cc: gcc-patches@gcc.gnu.org; Lu, Hongjiu; Guo, Xuepeng; Jan Hubicka;
jason@redhat.com; Ian Lance Taylor; Mark Mitchell
Subject: Re: [PATCH 2/7]: Ping3: Merge from Stack Branch - collect
alignment info

On Wed, May 28, 2008 at 11:00 AM, Ye, Joey <joey.ye@intel.com> wrote:
> Thanks for Richard's review:
> http://gcc.gnu.org/ml/gcc-patches/2008-05/msg01589.html
> http://gcc.gnu.org/ml/gcc-patches/2008-05/msg01654.html
>
> OK for mainline?

+         else
+           {
+             gcc_assert (!crtl->stack_realign_finalized
+                         && crtl->stack_realign_needed);
+           }

no braces needed here.

+  /* Assertion to check internal_arg_pointer is set to the right rtx
+     here.  */
+  gcc_assert (crtl->args.internal_arg_pointer ==
+             virtual_incoming_args_rtx);

the == goes on the next line.

Index: reload1.c
===================================================================
--- reload1.c   (revision 136046)
+++ reload1.c   (working copy)
...
-           reg_eliminate[i].can_eliminate = 0;
+            {
+             /* Must not disable reg eliminate because stack
realignment
+                must eliminate frame pointer to stack pointer.  */
+             gcc_assert (! MAX_STACK_ALIGNMENT
+                         || ! stack_realign_fp);
+             reg_eliminate[i].can_eliminate = 0;
+            }

there is a lot of changes like this in reload1.c.  Can you instead add
an inline function like

static inline
void set_not_eliminatable (struct elim_table *p)
{
  /* Must not disable reg eliminate because stack realignment
     must eliminate frame pointer to stack pointer.  */
    gcc_assert (! MAX_STACK_ALIGNMENT
                     || ! stack_realign_fp);
  p->can_eliminate = 0;
}

and use that?  That clutters reload1.c less.

Otherwise the patch looks good, but as it touches reload and I'm not
sure
this qualifies as a non-algorithmic change I think I cannot approve it.
I have CC'ed Ian and Mark to eventually have a look.

Thanks,
Richard.

> Thanks - Joey
>
> 2008-05-28  Joey Ye  <joey.ye@intel.com>
>            H.J. Lu  <hongjiu.lu@intel.com>
>
>        * builtins.c (expand_builtin_setjmp_receiver): Replace
>        virtual_incoming_args_rtx with
>        crtl->args.internal_arg_pointer.
>        (expand_builtin_apply_args_1): Likewise.
>        (expand_builtin_longjmp): DRAP will be needed if some builtins
> are
>        called.
>        (expand_builtin_apply): Likewise.
>
>        * calls.c (expand_call): Replace virtual_incoming_args_rtx with
>        crtl->args.internal_arg_pointer.
>        (emit_call_1): DRAP will be needed if return pops.
>
>        * caller-save.c (setup_save_areas): Call assign_stack_local_1
>        instead of assign_stack_local to allow alignment reduction.
>
>        * rtl.h (assign_stack_local_1): Declare new funtion.
>
>        * emit-rtl.c (gen_reg_rtx): Estimate stack alignment when
> generating
>        virtual registers.
>
>        * cfgexpand.c (get_decl_align_unit): Estimate stack variable
>        alignment and store to stack_alignment_estimated and
>        max_used_stack_slot_alignment.
>        (expand_one_var): Likewise.
>        (gate_handle_drap): Gate new pass pass_handle_drap.
>        (handle_drap): New function.
>        (tree_expand_cfg): Calls handle_drap at end. Initialize
rtl_data
>        fields.
>
>        * defaults.h (MAX_VECTORIZE_STACK_ALIGNMENT): New.
>
>        * function.c (assign_stack_local_1): Estimate stack variable
>        alignment and store to stack_alignment_estimated.
>        (instantiate_new_reg): Instantiate virtual incoming args rtx to
>        vDRAP if stack realignment and DRAP is needed.
>        (assign_parms): Collect parameter/return type alignment and
>        contribute to stack_alignment_estimated.
>        (locate_and_pad_parm): Likewise.
>        (allocate_struct_function): Init stack_alignment_estimated and
>        max_used_stack_slot_alignment.
>        (get_arg_pointer_save_area): Replace virtual_incoming_args_rtx
>        with crtl->args.internal_arg_pointer.
>        (LOCAL_ALIGNMENT): Removed.
>        (get_stack_local_alignment): New.
>        (assign_stack_local): Use it.  Set alignment on stack slot.
>        (assign_stack_temp_for_type): Use get_stack_local_alignment.
>
>
>        * global.c (compute_regsets): Set need_fp cannot_elim
>        wrt stack_realign_needed. Set frame_pointer_needed.
>
>
>        * stmt.c (expand_nl_goto_receiver): Replace
>        virtual_incoming_args_rtx with
>        crtl->args.internal_arg_pointer.
>
>        * tree-vectorizer.c (vect_can_force_dr_alignment_p): Return
>        true if alignment of variable on stack is less than or
>        equal to MAX_VECTORIZE_STACK_ALIGNMENT.
>
>        * reload1.c (set_label_offsets): Assert that frame pointer must
> be
>        elimiated to stack pointer in case stack realignment is
> estimated
>        to happen without DRAP.
>        (elimination_effects): Likewise.
>        (eliminate_regs_in_insn): Likewise.
>        (mark_not_eliminable): Likewise.
>        (update_eliminables): Likewise.
>        (init_elim_table): Don't set frame_pointer_needed here.
>

Attachment: reload-0528.patch
Description: reload-0528.patch