Discussion:
RFA: RTL typesafety improvements for ira.c
Jeff Law
2014-10-01 22:34:16 UTC
Permalink
This was inspired by a discussion with Felix who was making changes in
this area.

Basically this promotes the "init_insns" field within struct equivalence
from an rtx to an rtx_insn_list.

The only thing that's really interesting here is the old code exploits
the fact that we could put any RTX in the list by shoving const0_rtx
into the init_insns list as a marker to indicate we've already
determined the relevant pseudo must not have an equivalence.

Thus we could have the following objects in the init_insns field:

NULL
const0_rtx
INSN_LIST ...

This patch uses INSN_LIST (NULL_RTX, NULL) as the special marker. Thus
the only two things that would exist in the init_insns field would be
NULL or an INSN_LIST. Goodness.

Rather than stash away the special marker INSN_LIST into a global
variable or something similar, we instead to do a two step check for the
marker. First verify that the insn_list field is non-NULL, then look at
the first insn in the list and see if that is NULL.

Bootstrapped and regression tested on i686-unknown-linux-gnu and
x86_64-unknown-linux-gnu.

Ok for the trunk?

Jeff
David Malcolm
2014-10-01 23:27:14 UTC
Permalink
Post by Jeff Law
This was inspired by a discussion with Felix who was making changes in
this area.
Basically this promotes the "init_insns" field within struct equivalence
from an rtx to an rtx_insn_list.
The only thing that's really interesting here is the old code exploits
the fact that we could put any RTX in the list by shoving const0_rtx
into the init_insns list as a marker to indicate we've already
determined the relevant pseudo must not have an equivalence.
NULL
const0_rtx
INSN_LIST ...
This patch uses INSN_LIST (NULL_RTX, NULL) as the special marker. Thus
the only two things that would exist in the init_insns field would be
NULL or an INSN_LIST. Goodness.
Rather than stash away the special marker INSN_LIST into a global
variable or something similar, we instead to do a two step check for the
marker. First verify that the insn_list field is non-NULL, then look at
the first insn in the list and see if that is NULL.
Bootstrapped and regression tested on i686-unknown-linux-gnu and
x86_64-unknown-linux-gnu.
Ok for the trunk?
[...]
Post by Jeff Law
* ira.c (struct equivalence): Promote INIT_INSNs field to
an rtx_insn_list. Add comments.
(no_equiv): Promote LIST to an rtx_insn_list. Update
testing for and creating the special marker. Use methods
to extract the insn and next pointers.
(update_equiv_regs): Update test for special marker in the
INIT_INSNs list.
diff --git a/gcc/ira.c b/gcc/ira.c
[...]
Post by Jeff Law
@@ -3258,9 +3266,9 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSED,
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 = list->insn ();
remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
FWIW, presumably "insn" here also can now be an rtx_insn *?

(I'd like to eventually strengthen the params to the note-handling
functions, so fixing this up now would help with that).
Post by Jeff Law
}
[...]
Jeff Law
2014-10-02 18:03:55 UTC
Permalink
Post by David Malcolm
Post by Jeff Law
This was inspired by a discussion with Felix who was making changes in
this area.
Basically this promotes the "init_insns" field within struct equivalence
from an rtx to an rtx_insn_list.
The only thing that's really interesting here is the old code exploits
the fact that we could put any RTX in the list by shoving const0_rtx
into the init_insns list as a marker to indicate we've already
determined the relevant pseudo must not have an equivalence.
NULL
const0_rtx
INSN_LIST ...
This patch uses INSN_LIST (NULL_RTX, NULL) as the special marker. Thus
the only two things that would exist in the init_insns field would be
NULL or an INSN_LIST. Goodness.
Rather than stash away the special marker INSN_LIST into a global
variable or something similar, we instead to do a two step check for the
marker. First verify that the insn_list field is non-NULL, then look at
the first insn in the list and see if that is NULL.
Bootstrapped and regression tested on i686-unknown-linux-gnu and
x86_64-unknown-linux-gnu.
Ok for the trunk?
[...]
Post by Jeff Law
* ira.c (struct equivalence): Promote INIT_INSNs field to
an rtx_insn_list. Add comments.
(no_equiv): Promote LIST to an rtx_insn_list. Update
testing for and creating the special marker. Use methods
to extract the insn and next pointers.
(update_equiv_regs): Update test for special marker in the
INIT_INSNs list.
diff --git a/gcc/ira.c b/gcc/ira.c
[...]
Post by Jeff Law
@@ -3258,9 +3266,9 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSED,
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 = list->insn ();
remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
FWIW, presumably "insn" here also can now be an rtx_insn *?
Yes. I'll update that. Thanks.

Jeff
Jeff Law
2014-10-09 02:21:47 UTC
Permalink
Post by David Malcolm
FWIW, presumably "insn" here also can now be an rtx_insn *?
(I'd like to eventually strengthen the params to the note-handling
functions, so fixing this up now would help with that).
Here's the updated patch to include strengthening insn to rtx_insn *.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu.

OK for the trunk? I'd like to get this wrapped up so that a patch in
the same area from Felix can get rebased and move forward.

Jeff

Loading...