[PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition
Felix Yang
fei.yang0953@gmail.com
Sat Sep 27 14:48:00 GMT 2014
Thanks for the explaination.
I have changed the loop_depth into a short interger hoping that we can
save some memory :-)
Attached please find the updated patch. Bootstrapped and reg-tested on
x86_64-suse-linux.
Please do a final revew once the assignment is ready.
As for the new list walking interface, I choose the function
"no_equiv" and tried the "checked cast" way.
The bad news is that GCC failed to bootstrap with the following change:
Index: ira.c
===================================================================
--- ira.c (revision 215536)
+++ ira.c (working copy)
@@ -3242,12 +3242,12 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
void *data ATTRIBUTE_UNUSED)
{
int regno;
- rtx list;
+ rtx_insn_list *list;
if (!REG_P (reg))
return;
regno = REGNO (reg);
- list = reg_equiv[regno].init_insns;
+ list = as_a <rtx_insn_list *> (reg_equiv[regno].init_insns);
if (list == const0_rtx)
return;
reg_equiv[regno].init_insns = const0_rtx;
@@ -3258,9 +3258,9 @@ 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 = list->next ())
{
- rtx insn = XEXP (list, 0);
+ rtx_insn *insn = list->insn ();
remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
}
}
Error message:
.... ...
checking for suffix of object files... configure: error: in
`/home/yangfei/gcc-devel/build/x86_64-unknown-linux-gnu/libgcc':
configure: error: cannot compute suffix of object files: cannot compile
See `config.log' for more details.
make[2]: *** [configure-stage1-target-libgcc] Error 1
make[2]: Leaving directory `/home/yangfei/gcc-devel/build'
make[1]: *** [stage1-bubble] Error 2
make[1]: Leaving directory `/home/yangfei/gcc-devel/build'
make: *** [all] Error 2
I think the code change is OK. Anything I missed?
Cheers,
Felix
On Sat, Sep 27, 2014 at 5:03 AM, Jeff Law <law@redhat.com> wrote:
> On 09/26/14 07:57, Felix Yang wrote:
>>
>> Hi Jeff,
>>
>> Thanks for the suggestions. I updated the patch accordingly.
>>
>> 1. Both my employer(Huawei) and I have signed the copyright
>> assignments with FSF.
>> These assignments are already sent via post two days ago and
>> hopefully should reach FSF in one week.
>> Maybe it's OK to commit this patch now?
>
> Not really. It needs to be accepted by the FSF before we can include the
> work.
>
>>
>> 2. I am not turning member loop_depth of struct equivalence into
>> short integer as GCC API such as bb_loop_depth
>> returns a loop's depth as a 32-bit interger.
>
> There's already other places that assume loops don't nest that deep. Please
> go ahead and change it. And no need to explicitly mark the unused bits.
> That's just a maintenance nightmare in the long term anyway :-)
>
>
>>
>> 3. I find it's kind of difficult to use the new type and
>> interfaces for list walking the init_insns list for this patch.
>> The type of init_insns list is rtx, not rtl_insn_list *. Seems
>> we need to change a lot in order to use the new interface.
>> Not clear about the reason why it is not adjusted when we are
>> transferring to the new interface.
>> Anyway, I think it's better to have another patch fix that issue.
>> OK?
>
> The right way to go is to add a checked cast when we have some code that is
> using the old interface and other code using the new interface. It's
> actually a pretty easy change.
>
> The checked casts effectively mark the limits of where we've been able to
> push the RTL typesafety work. Long term as we push the typesafety work
> further into the compiler many/most of the checked casts will go away.
>
> Unfortunately, that won't work in this case because other code wants to
> store a (const0_rtx) into the insn list. (const0_rtx) isn't an INSN, so the
> checked cast fails and we get a nice abort/ICE.
>
> Conceptually we just need another marker that is an INSN and we might as
> well just convert the whole file to use the new interface at that point.
>
> Consider the request pulled.
>
> The const0-rtx problem may be why this wasn't converted in the first palce.
> Or it may simply have been a time problem. David's done > 250 patches
> around RTL typesafety, but he also has other work to be doing ;-)
>
>>
>> 4. This bug is only reproduceable with my local customized GCC
>> version. So I don't have a testcase then.
>
> OK.
>
> I'll do a final review when I get notice about the copyright assignment from
> the FSF.
>
> jeff
>
-------------- next part --------------
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog (revision 215658)
+++ gcc/ChangeLog (working copy)
@@ -1,3 +1,14 @@
+2014-09-26 Felix Yang <felix.yang@huawei.com>
+ Jeff Law <law@redhat.com>
+
+ * ira.c (struct equivalence): Change member "is_arg_equivalence" and "replace"
+ into boolean bitfields; turn member "loop_depth" into a short integer; add new
+ member "no_equiv" and "reserved".
+ (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-26 Jan Hubicka <hubicka@ucw.cz>
PR ipa/60665
Index: gcc/ira.c
===================================================================
--- gcc/ira.c (revision 215658)
+++ gcc/ira.c (working copy)
@@ -2894,12 +2894,14 @@ struct equivalence
rtx init_insns;
/* Loop depth is used to recognize equivalences which appear
to be present within the same loop (or in an inner loop). */
- int loop_depth;
+ short loop_depth;
/* Nonzero if this had a preexisting REG_EQUIV note. */
- int is_arg_equivalence;
+ unsigned char is_arg_equivalence : 1;
/* Set when an attempt should be made to replace a register
with the associated src_p entry. */
- char replace;
+ unsigned char replace : 1;
+ /* Set if this register has no known equivalence. */
+ unsigned char no_equiv : 1;
};
/* 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;
+
+ /* If we have already processed this pseudo and determined it
+ can not have an equivalence, then honor that decision. */
+ 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)));
@@ -3538,7 +3572,7 @@ update_equiv_regs (void)
reg_equiv[regno].replacement = x;
reg_equiv[regno].src_p = &SET_SRC (set);
- reg_equiv[regno].loop_depth = loop_depth;
+ reg_equiv[regno].loop_depth = (short) loop_depth;
/* Don't mess with things live during setjmp. */
if (REG_LIVE_LENGTH (regno) >= 0 && optimize)
@@ -3668,7 +3702,7 @@ update_equiv_regs (void)
rtx equiv_insn;
if (! reg_equiv[regno].replace
- || reg_equiv[regno].loop_depth < loop_depth
+ || reg_equiv[regno].loop_depth < (short) loop_depth
/* There is no sense to move insns if live range
shrinkage or register pressure-sensitive
scheduling were done because it will not
More information about the Gcc-patches
mailing list