This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
- From: Felix Yang <fei dot yang0953 at gmail dot com>
- To: Jeff Law <law at redhat dot com>
- Cc: "Yangfei (Felix)" <felix dot yang at huawei dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>, "vmakarov at redhat dot com" <vmakarov at redhat dot com>
- Date: Thu, 25 Sep 2014 05:56:39 +0800
- Subject: Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
- Authentication-results: sourceware.org; auth=none
- References: <CAFc0fxz3Fdr9rgLscjTzZm0PoBn53y028tQU4=U0BXQ0kY6+KA at mail dot gmail dot com> <54206D2C dot 708 at redhat dot com> <DA41BE1DDCA941489001C7FBD7A8820E55542982 at szxema507-mbx dot china dot huawei dot com> <CAFc0fxzT7hTBONNt+_2EmQwW8jTmadPs6oWnDMRRD85=yUqzNg at mail dot gmail dot com> <CAFc0fxyu257uDv4twJiFUGd-g60NZLGyDNyQthDHwtwvPaXRpQ at mail dot gmail dot com> <5421B28F dot 1030006 at redhat dot com> <CAFc0fxx-3MA+MaeWK8eXktOP8+s3NPQMCUNvytmwJJ8FXXWQwA at mail dot gmail dot com>
PING ?
Cheers,
Felix
On Wed, Sep 24, 2014 at 8:07 PM, Felix Yang <fei.yang0953@gmail.com> wrote:
> Hi Jeff,
>
> Thanks for the comments. I updated the patch adding some enhancements.
> Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for trunk.
>
> Three points:
> 1. For multiple-set register, it is not qualified to have a equiv
> note once it is marked by no_equiv. The patch is updated with
> this consideration.
> 2. For the rtx_insn_list new interface, I noticed that the old
> style XEXP accessor macros is still used in function no_equiv.
> And I choose to the old style macros with this patch and should
> come up with another patch to fix this issue, OK?
> 3. For the conditions that an insn on the init_insns list which
> did not have a note, I reconsider this and find that this can
> never happens. So I replaced the check with a gcc assertion.
>
>
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog (revision 215550)
> +++ gcc/ChangeLog (working copy)
> @@ -1,3 +1,11 @@
> +2014-09-24 Felix Yang <felix.yang@huawei.com>
> +
> + * ira.c (struct equivalence): Add no_equiv member.
> + (no_equiv): Set no_equiv of struct equivalence if register is marked
> + as having no known equivalence.
> + (update_equiv_regs): Check all definitions for a multiple-set
> + register to make sure that the RHS have the same value.
> +
> 2014-09-24 Jakub Jelinek <jakub@redhat.com>
>
> PR sanitizer/63316
> Index: gcc/ira.c
> ===================================================================
> --- gcc/ira.c (revision 215550)
> +++ gcc/ira.c (working copy)
> @@ -2900,6 +2900,8 @@ struct equivalence
> /* Set when an attempt should be made to replace a register
> with the associated src_p entry. */
> char replace;
> + /* Set if this register has no known equivalence. */
> + char no_equiv;
> };
>
> /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
> @@ -3247,6 +3249,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
> if (!REG_P (reg))
> return;
> regno = REGNO (reg);
> + reg_equiv[regno].no_equiv = 1;
> list = reg_equiv[regno].init_insns;
> if (list == const0_rtx)
> return;
> @@ -3258,7 +3261,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
> return;
> ira_reg_equiv[regno].defined_p = false;
> ira_reg_equiv[regno].init_insns = NULL;
> - for (; list; list = XEXP (list, 1))
> + for (; list; list = XEXP (list, 1))
> {
> rtx insn = XEXP (list, 0);
> remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
> @@ -3373,7 +3376,7 @@ update_equiv_regs (void)
>
> /* If this insn contains more (or less) than a single SET,
> only mark all destinations as having no known equivalence. */
> - if (set == 0)
> + if (set == NULL_RTX)
> {
> note_stores (PATTERN (insn), no_equiv, NULL);
> continue;
> @@ -3467,16 +3470,48 @@ update_equiv_regs (void)
> if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
> note = NULL_RTX;
>
> - if (DF_REG_DEF_COUNT (regno) != 1
> - && (! note
> + if (DF_REG_DEF_COUNT (regno) != 1)
> + {
> + rtx list;
> + bool equal_p = true;
> +
> + /* Check if it is possible that this multiple-set register has
> + a known equivalence. */
> + if (reg_equiv[regno].no_equiv)
> + continue;
> +
> + if (! note
> || rtx_varies_p (XEXP (note, 0), 0)
> || (reg_equiv[regno].replacement
> && ! rtx_equal_p (XEXP (note, 0),
> - reg_equiv[regno].replacement))))
> - {
> - no_equiv (dest, set, NULL);
> - continue;
> + reg_equiv[regno].replacement)))
> + {
> + no_equiv (dest, set, NULL);
> + continue;
> + }
> +
> + list = reg_equiv[regno].init_insns;
> + for (; list; list = XEXP (list, 1))
> + {
> + rtx note_tmp, insn_tmp;
> +
> + insn_tmp = XEXP (list, 0);
> + note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
> + gcc_assert (note_tmp);
> + if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
> + {
> + equal_p = false;
> + break;
> + }
> + }
> +
> + if (! equal_p)
> + {
> + no_equiv (dest, set, NULL);
> + continue;
> + }
> }
> +
> /* Record this insn as initializing this register. */
> reg_equiv[regno].init_insns
> = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
> @@ -3505,10 +3540,9 @@ update_equiv_regs (void)
> a register used only in one basic block from a MEM. If so, and the
> MEM remains unchanged for the life of the register, add a REG_EQUIV
> note. */
> -
> note = find_reg_note (insn, REG_EQUIV, NULL_RTX);
>
> - if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
> + if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
> && MEM_P (SET_SRC (set))
> && validate_equiv_mem (insn, dest, SET_SRC (set)))
> note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)));
> Cheers,
> Felix
>
>
> On Wed, Sep 24, 2014 at 1:49 AM, Jeff Law <law@redhat.com> wrote:
>> On 09/23/14 04:51, Felix Yang wrote:
>>>
>>> Hi,
>>>
>>> Ignore the previous message.
>>> Attached please find the updated patch.
>>> Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for
>>> trunk.
>>>
>>> Index: gcc/ChangeLog
>>> ===================================================================
>>> --- gcc/ChangeLog (revision 215500)
>>> +++ gcc/ChangeLog (working copy)
>>> @@ -1,3 +1,8 @@
>>> +2014-09-23 Felix Yang <felix.yang@huawei.com>
>>> +
>>> + * ira.c (update_equiv_regs): Check all definitions for a multiple-set
>>> + register to make sure that the RHS have the same value.
>>> +
>>> 2014-09-23 Ilya Enkovich <ilya.enkovich@intel.com>
>>>
>>> * cfgcleanup.c (try_optimize_cfg): Do not remove label
>>> Index: gcc/ira.c
>>> ===================================================================
>>> --- gcc/ira.c (revision 215500)
>>> +++ gcc/ira.c (working copy)
>>> @@ -3467,16 +3467,43 @@ update_equiv_regs (void)
>>> if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
>>> note = NULL_RTX;
>>>
>>> - if (DF_REG_DEF_COUNT (regno) != 1
>>> - && (! note
>>> + if (DF_REG_DEF_COUNT (regno) != 1)
>>> + {
>>> + rtx list;
>>
>> This should probably be "rtx_insn_list list".
>>
>>> + list = reg_equiv[regno].init_insns;
>>> + for (; list; list = XEXP (list, 1))
>>
>> Please use the next/insn member functions of the rtx_insn_list rather than
>> the old style XEXP accessor macros.
>>
>>
>>> + {
>>> + rtx note_tmp, insn_tmp;
>>> + insn_tmp = XEXP (list, 0);
>>> + note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
>>> +
>>> + if (note_tmp == 0
>>> + || ! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
>>
>> Under what conditions did you find an insn on this list that did not have a
>> note? There's a deeper question I'm getting to, but let's start here.
>>
>> Rather than use "== 0", if you have an RTX use "== NULL_RTX". That applies
>> to the note_tmp == 0 test above.
>>
>>
>> Can you make the edits noted above & answer the question WRT insns on the
>> reg_equiv[].init_insns list without notes and repost?
>>
>> Thanks,
>> Jeff