This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, i386, Pointer Bounds Checker 33/x] MPX ABI
- From: Ilya Enkovich <enkovich dot gnu at gmail dot com>
- To: Uros Bizjak <ubizjak at gmail dot com>
- Cc: Vladimir Makarov <vmakarov at redhat dot com>, Jeff Law <law at redhat dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Fri, 10 Oct 2014 21:29:49 +0400
- Subject: Re: [PATCH, i386, Pointer Bounds Checker 33/x] MPX ABI
- Authentication-results: sourceware.org; auth=none
- References: <CAFULd4a4JiD4Z8ygzvtODWaNtEz8pZbXYO+rBkWUL3MXv6ySOQ at mail dot gmail dot com> <20140919075558 dot GE50194 at msticlxl57 dot ims dot intel dot com> <CAFULd4ZSE3y=yf4Yf1+9qYbK5KzNs61qehyw-qokhLwAJm+=LA at mail dot gmail dot com> <542068E1 dot 5060504 at redhat dot com> <CAMbmDYZu9NpXsVn+DWQckcKhj6zR-Fh_EQ10FhdiQ03hqgxEgQ at mail dot gmail dot com> <5421B591 dot 6090705 at redhat dot com> <CAMbmDYahdc32CZvQzFGP33dQVFEALUTzRmBuMdyXSidPaUrrTg at mail dot gmail dot com> <CAMbmDYZX1+8F+YOODkPk_Pm7dfLaXRHLAvStem-unDwq+P7qCw at mail dot gmail dot com> <542316DC dot 5030700 at redhat dot com> <CAMbmDYZ0=yhFusghp5hupzs8SWoa1XEF1qrSA3gW=e2xHbmHhg at mail dot gmail dot com> <CAMbmDYbzqhiGTgnnBzAs=G1eBQUXnTn-qHALyjLYNUqwwARHXw at mail dot gmail dot com> <542C4E8F dot 8030502 at redhat dot com> <CAFULd4bnH2cc+yFYq74eaN+Z286bFx3p-hgHjDs=Di+68OoJ6A at mail dot gmail dot com>
2014-10-10 21:10 GMT+04:00 Uros Bizjak <ubizjak@gmail.com>:
> 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.
The first solutions is in trunk since October 3
(https://gcc.gnu.org/ml/gcc-cvs/2014-10/msg00094.html) and I don't see
such fail. Patch actually just checks for a case which never occurs
right now and therefore should be quite safe.
Ilya
>
>> 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.