diff mbox

RFA: RTL typesafety improvements for ira.c

Message ID 542C8168.4030409@redhat.com
State New
Headers show

Commit Message

Jeff Law Oct. 1, 2014, 10:34 p.m. UTC
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
* 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.

Comments

David Malcolm Oct. 1, 2014, 11:27 p.m. UTC | #1
On Wed, 2014-10-01 at 16:34 -0600, Jeff Law wrote:
> 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?
> 

[...]

> 	* 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
[...]

> @@ -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).

>      }

[...]
Jeff Law Oct. 2, 2014, 6:03 p.m. UTC | #2
On 10/01/14 17:27, David Malcolm wrote:
> On Wed, 2014-10-01 at 16:34 -0600, Jeff Law wrote:
>> 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?
>>
>
> [...]
>
>> 	* 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
> [...]
>
>> @@ -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
diff mbox

Patch

diff --git a/gcc/ira.c b/gcc/ira.c
index ad0e463..231cfc5 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -2890,8 +2890,16 @@  struct equivalence
      e.g. by reload.  */
   rtx replacement;
   rtx *src_p;
-  /* The list of each instruction which initializes this register.  */
-  rtx init_insns;
+
+  /* The list of each instruction which initializes this register. 
+
+     NULL indicates we know nothing about this register's equivalence
+     properties.
+
+     An INSN_LIST with a NULL insn indicates this pseudo is already
+     known to not have a valid equivalence.  */
+  rtx_insn_list *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;
@@ -3242,15 +3250,15 @@  no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSED,
 	  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;
-  if (list == const0_rtx)
+  if (list && list->insn () == NULL)
     return;
-  reg_equiv[regno].init_insns = const0_rtx;
+  reg_equiv[regno].init_insns = gen_rtx_INSN_LIST (VOIDmode, NULL_RTX, NULL);
   reg_equiv[regno].replacement = NULL_RTX;
   /* This doesn't matter for equivalences made for argument registers, we
      should keep their initialization insns.  */
@@ -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));
     }
 }
@@ -3437,7 +3445,8 @@  update_equiv_regs (void)
 
 	  if (!REG_P (dest)
 	      || (regno = REGNO (dest)) < FIRST_PSEUDO_REGISTER
-	      || reg_equiv[regno].init_insns == const0_rtx
+	      || (reg_equiv[regno].init_insns
+		  && reg_equiv[regno].init_insns->insn () == NULL)
 	      || (targetm.class_likely_spilled_p (reg_preferred_class (regno))
 		  && MEM_P (src) && ! reg_equiv[regno].is_arg_equivalence))
 	    {
@@ -3608,8 +3617,8 @@  update_equiv_regs (void)
 	  && (regno = REGNO (src)) >= FIRST_PSEUDO_REGISTER
 	  && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
 	  && DF_REG_DEF_COUNT (regno) == 1
-	  && reg_equiv[regno].init_insns != 0
-	  && reg_equiv[regno].init_insns != const0_rtx
+	  && reg_equiv[regno].init_insns != NULL
+	  && reg_equiv[regno].init_insns->insn () != NULL
 	  && ! find_reg_note (XEXP (reg_equiv[regno].init_insns, 0),
 			      REG_EQUIV, NULL_RTX)
 	  && ! contains_replace_regs (XEXP (dest, 0))
@@ -3728,7 +3737,7 @@  update_equiv_regs (void)
 		      delete_insn (equiv_insn);
 
 		      reg_equiv[regno].init_insns
-			= XEXP (reg_equiv[regno].init_insns, 1);
+			= reg_equiv[regno].init_insns->next ();
 
 		      ira_reg_equiv[regno].init_insns = NULL;
 		      bitmap_set_bit (cleared_regs, regno);