This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, i386, Pointer Bounds Checker 33/x] MPX ABI


On Wed, Oct 1, 2014 at 8:57 PM, Vladimir Makarov <vmakarov@redhat.com> wrote:

> The problem is in code introduced by Bernd in IRA and caller-saves.c in
> 2012.  It is basically an optimization for functions returning always the
> same result as one argument (e.g. memcpy returning 1st argument).
>
> There are two possible solutions.  First one is to prohibit the
> optimizations when there is a parallel in SET.  Second one is to go deeper
> if the call result is guaranteed in the first element which is true for the
> patch.

I suspect that the first solution will regress
gcc.target/i386/retarg.c on i686 - that testcase test if referred
optimization is effective. All things equal, I think we should go with
the second solution.

> For the first solution, the patch would be
>
> Index: lra-constraints.c
> ===================================================================
> --- lra-constraints.c   (revision 215748)
> +++ lra-constraints.c   (working copy)
> @@ -5348,16 +5348,19 @@
>                   if (GET_CODE (pat) == PARALLEL)
>                     pat = XVECEXP (pat, 0, 0);
>                   dest = SET_DEST (pat);
> -                 start_sequence ();
> -                 emit_move_insn (cheap, copy_rtx (dest));
> -                 restore = get_insns ();
> -                 end_sequence ();
> -                 lra_process_new_insns (curr_insn, NULL, restore,
> -                                        "Inserting call parameter
> restore");
> -                 /* We don't need to save/restore of the pseudo from
> -                    this call.  */
> -                 usage_insns[regno].calls_num = calls_num;
> -                 bitmap_set_bit (&check_only_regs, regno);
> +                 if (REG_P (dest))
> +                   {
> +                     start_sequence ();
> +                     emit_move_insn (cheap, copy_rtx (dest));
> +                     restore = get_insns ();
> +                     end_sequence ();
> +                     lra_process_new_insns (curr_insn, NULL, restore,
> +                                            "Inserting call parameter
> restore");
> +                     /* We don't need to save/restore of the pseudo
> +                        from this call.  */
> +                     usage_insns[regno].calls_num = calls_num;
> +                     bitmap_set_bit (&check_only_regs, regno);
> +                   }
>                 }
>             }
>           to_inherit_num = 0;
>
>
> For the second solution, the patch is
>
>
> Index: lra-constraints.c
> ===================================================================
> --- lra-constraints.c   (revision 215748)
> +++ lra-constraints.c   (working copy)
> @@ -5348,16 +5348,25 @@
>                   if (GET_CODE (pat) == PARALLEL)
>                     pat = XVECEXP (pat, 0, 0);
>                   dest = SET_DEST (pat);
> -                 start_sequence ();
> -                 emit_move_insn (cheap, copy_rtx (dest));
> -                 restore = get_insns ();
> -                 end_sequence ();
> -                 lra_process_new_insns (curr_insn, NULL, restore,
> -                                        "Inserting call parameter
> restore");
> -                 /* We don't need to save/restore of the pseudo from
> -                    this call.  */
> -                 usage_insns[regno].calls_num = calls_num;
> -                 bitmap_set_bit (&check_only_regs, regno);
> +                 if (GET_CODE (dest) == PARALLEL)
> +                   {
> +                     dest = XVECEXP (dest, 0, 0);
> +                     if (GET_CODE (dest) == EXPR_LIST)
> +                       dest = XEXP (dest, 0);
> +                   }
> +                 if (REG_P (dest))
> +                   {
> +                     start_sequence ();
> +                     emit_move_insn (cheap, copy_rtx (dest));
> +                     restore = get_insns ();
> +                     end_sequence ();
> +                     lra_process_new_insns (curr_insn, NULL, restore,
> +                                            "Inserting call parameter
> restore");
> +                     /* We don't need to save/restore of the pseudo from
> +                        this call.      */
> +                     usage_insns[regno].calls_num = calls_num;
> +                     bitmap_set_bit (&check_only_regs, regno);
> +                   }
>                 }
>             }
>
>
> The first patch is safer but the second one is ok too.  I have no particular
> preferences.  Whatever we choose, analogous code in caller-saves.c should be
> changed too.

Uros.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]