Patchwork [lra] patch to fix testsuite regressions

login
register
mail settings
Submitter Vladimir Makarov
Date Oct. 20, 2012, 3:46 p.m.
Message ID <5082C740.3020706@redhat.com>
Download mbox | patch
Permalink /patch/192923/
State New
Headers show

Comments

Vladimir Makarov - Oct. 20, 2012, 3:46 p.m.
After recent patches there were too many regressions of LRA on GCC 
testsuite on x86 and x86-64.

   The following patch fixes all of them.

   It was successfully bootstrapped on x86/x86-64.

   Committed as rev. 192637.

2012-10-20  Vladimir Makarov  <vmakarov@redhat.com>

     * lra.c (check_rtx): Don't check UNSPEC address.  Fix typo with
     comparing RTX_AUTOINC.
     * lra-constraints.c (extract_local_address): Swap operands if
     necessary.  Assign to disp before extract_local_address call.  Fix
     typo with PLUS comparison.  Use CONSTANT_P.
     (simplify_operand_subreg): Put constant into memory for subreg
     with mixed modes.
     (process_alt_operands): Uncomment code for checking DEAD not for
     early clobber.
     * config/i386/i386.c (ix86_spill_class): Don't spill to SSE
     regs when TARGET_MMX.
Richard Sandiford - Oct. 21, 2012, 9:27 a.m.
Hi Vlad,

Vladimir Makarov <vmakarov@redhat.com> writes:
> @@ -543,23 +544,34 @@ extract_loc_address_regs (bool top_p, en
>  	    code1 = GET_CODE (arg1);
>  	  }
>  
> +	if (CONSTANT_P (arg0)
> +	    || code1 == PLUS || code1 == MULT || code1 == ASHIFT)
> +	  {
> +	    tloc = arg1_loc;
> +	    arg1_loc = arg0_loc;
> +	    arg0_loc = tloc;
> +	    arg0 = *arg0_loc;
> +	    code0 = GET_CODE (arg0);
> +	    arg1 = *arg1_loc;
> +	    code1 = GET_CODE (arg1);
> +	  }

When does this happen?  Is it from the extract_address_regs calls
in equiv_address_substitution, or somewhere else?

>  	/* If this machine only allows one register per address, it
>  	   must be in the first operand.  */
>  	if (MAX_REGS_PER_ADDRESS == 1 || code == LO_SUM)
>  	  {
> -	    extract_loc_address_regs (false, mode, as, arg0_loc, false, code,
> -				      code1, modify_p, ad);
>  	    lra_assert (ad->disp_loc == NULL);
>  	    ad->disp_loc = arg1_loc;
> +	    extract_loc_address_regs (false, mode, as, arg0_loc, false, code,
> +				      code1, modify_p, ad);
>  	  }
>  	/* Base + disp addressing  */
> -	else if (code != PLUS && code0 != MULT && code0 != ASHIFT
> +	else if (code0 != PLUS && code0 != MULT && code0 != ASHIFT
>  		 && CONSTANT_P (arg1))
>  	  {
> -	    extract_loc_address_regs (false, mode, as, arg0_loc, false, PLUS,
> -				      code1, modify_p, ad);
>  	    lra_assert (ad->disp_loc == NULL);
>  	    ad->disp_loc = arg1_loc;
> +	    extract_loc_address_regs (false, mode, as, arg0_loc, false, PLUS,
> +				      code1, modify_p, ad);
>  	  }
>  	/* If index and base registers are the same on this machine,
>  	   just record registers in any non-constant operands.	We

Wasn't sure: is the order of the disp_loc assignment and
extract_loc_address_regs call significant here, or did you just swap
them for cosmetic reasons?  The original order seemed stronger on the
face of it, because we assert that the recursive call hasn't also
installed a displacement.

> @@ -624,11 +635,18 @@ extract_loc_address_regs (bool top_p, en
>  
>      case MULT:
>      case ASHIFT:
> -      extract_loc_address_regs (false, mode, as, &XEXP (*loc, 0), true,
> -				outer_code, code, modify_p, ad);
> -      lra_assert (ad->index_loc == NULL);
> -      ad->index_loc = loc;
> -      break;
> +      {
> +	rtx *arg0_loc = &XEXP (x, 0);
> +	enum rtx_code code0 = GET_CODE (*arg0_loc);
> +	
> +	if (code0 == CONST_INT)
> +	  arg0_loc = &XEXP (x, 1);
> +	extract_loc_address_regs (false, mode, as, arg0_loc, true,
> +				  outer_code, code, modify_p, ad);
> +	lra_assert (ad->index_loc == NULL);
> +	ad->index_loc = loc;
> +	break;
> +      }

Is this just for the MULT case?  Treating X in (ashift 1 X) as an
index register seems odd on the face of it.

> @@ -1757,10 +1786,10 @@ process_alt_operands (int only_alternati
>  			   clobber operand if the matching operand is
>  			   not dying in the insn.  */
>  			if (! curr_static_id->operand[m].early_clobber
> -			    /*|| operand_reg[nop] == NULL_RTX
> +			    || operand_reg[nop] == NULL_RTX
>  			    || (find_regno_note (curr_insn, REG_DEAD,
>  						 REGNO (operand_reg[nop]))
> -						 != NULL_RTX)*/)
> +						 != NULL_RTX))
>  			  match_p = true;
>  		      }
>  		    if (match_p)

Ah, so did you find the reason why this was done? :-)

> Index: lra.c
> ===================================================================
> --- lra.c	(revision 192634)
> +++ lra.c	(working copy)
> @@ -2035,7 +2035,8 @@ check_rtl (bool final_p)
>  		   as legitimate.  Although they are legitimate if
>  		   they satisfies the constraints and will be checked
>  		   by insn constraints which we ignore here.  */
> -		&& GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) == RTX_AUTOINC)
> +		&& GET_CODE (XEXP (op, 0)) != UNSPEC
> +		&& GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) != RTX_AUTOINC)
>  	      fatal_insn_not_found (insn);
>  	  }
>        }

Which case was this needed for?  RTX_AUTOINC codes really did seem like
a special case because they have their own constraints ("<" and ">").
But why do we have to assume that all unspec addresses are inherently valid?

Richard
Vladimir Makarov - Oct. 23, 2012, 8:10 p.m.
On 10/21/2012 05:27 AM, Richard Sandiford wrote:
> Hi Vlad,
Richard, sorry for long delay with the answer.  I was really busy all 
these days trying to fix a lot of GCC testsuite failures (for some ones 
it was extremely difficult to find failure reasons) and push LRA into trunk.

> Vladimir Makarov <vmakarov@redhat.com> writes:
>> @@ -543,23 +544,34 @@ extract_loc_address_regs (bool top_p, en
>>   	    code1 = GET_CODE (arg1);
>>   	  }
>>   
>> +	if (CONSTANT_P (arg0)
>> +	    || code1 == PLUS || code1 == MULT || code1 == ASHIFT)
>> +	  {
>> +	    tloc = arg1_loc;
>> +	    arg1_loc = arg0_loc;
>> +	    arg0_loc = tloc;
>> +	    arg0 = *arg0_loc;
>> +	    code0 = GET_CODE (arg0);
>> +	    arg1 = *arg1_loc;
>> +	    code1 = GET_CODE (arg1);
>> +	  }
> When does this happen?  Is it from the extract_address_regs calls
> in equiv_address_substitution, or somewhere else?
I did not analyzed this.  Sorry, I had no time for this.  I just saw it 
on a broken test.
I think the future work would be a support of canonical expressions 
through all LRA and machine depended code and also address extract 
rewriting when we figure out the exact syntax of the addresses.
>>   	/* If this machine only allows one register per address, it
>>   	   must be in the first operand.  */
>>   	if (MAX_REGS_PER_ADDRESS == 1 || code == LO_SUM)
>>   	  {
>> -	    extract_loc_address_regs (false, mode, as, arg0_loc, false, code,
>> -				      code1, modify_p, ad);
>>   	    lra_assert (ad->disp_loc == NULL);
>>   	    ad->disp_loc = arg1_loc;
>> +	    extract_loc_address_regs (false, mode, as, arg0_loc, false, code,
>> +				      code1, modify_p, ad);
>>   	  }
>>   	/* Base + disp addressing  */
>> -	else if (code != PLUS && code0 != MULT && code0 != ASHIFT
>> +	else if (code0 != PLUS && code0 != MULT && code0 != ASHIFT
>>   		 && CONSTANT_P (arg1))
>>   	  {
>> -	    extract_loc_address_regs (false, mode, as, arg0_loc, false, PLUS,
>> -				      code1, modify_p, ad);
>>   	    lra_assert (ad->disp_loc == NULL);
>>   	    ad->disp_loc = arg1_loc;
>> +	    extract_loc_address_regs (false, mode, as, arg0_loc, false, PLUS,
>> +				      code1, modify_p, ad);
>>   	  }
>>   	/* If index and base registers are the same on this machine,
>>   	   just record registers in any non-constant operands.	We
> Wasn't sure: is the order of the disp_loc assignment and
> extract_loc_address_regs call significant here, or did you just swap
> them for cosmetic reasons?  The original order seemed stronger on the
> face of it, because we assert that the recursive call hasn't also
> installed a displacement.
It was not a cosmetic change.  It was a necessary fix for a broken test.
>> @@ -624,11 +635,18 @@ extract_loc_address_regs (bool top_p, en
>>   
>>       case MULT:
>>       case ASHIFT:
>> -      extract_loc_address_regs (false, mode, as, &XEXP (*loc, 0), true,
>> -				outer_code, code, modify_p, ad);
>> -      lra_assert (ad->index_loc == NULL);
>> -      ad->index_loc = loc;
>> -      break;
>> +      {
>> +	rtx *arg0_loc = &XEXP (x, 0);
>> +	enum rtx_code code0 = GET_CODE (*arg0_loc);
>> +	
>> +	if (code0 == CONST_INT)
>> +	  arg0_loc = &XEXP (x, 1);
>> +	extract_loc_address_regs (false, mode, as, arg0_loc, true,
>> +				  outer_code, code, modify_p, ad);
>> +	lra_assert (ad->index_loc == NULL);
>> +	ad->index_loc = loc;
>> +	break;
>> +      }
> Is this just for the MULT case?  Treating X in (ashift 1 X) as an
> index register seems odd on the face of it.
Yes, right.  It is really safe but it should be fixed for code clearness.
>> @@ -1757,10 +1786,10 @@ process_alt_operands (int only_alternati
>>   			   clobber operand if the matching operand is
>>   			   not dying in the insn.  */
>>   			if (! curr_static_id->operand[m].early_clobber
>> -			    /*|| operand_reg[nop] == NULL_RTX
>> +			    || operand_reg[nop] == NULL_RTX
>>   			    || (find_regno_note (curr_insn, REG_DEAD,
>>   						 REGNO (operand_reg[nop]))
>> -						 != NULL_RTX)*/)
>> +						 != NULL_RTX))
>>   			  match_p = true;
>>   		      }
>>   		    if (match_p)
> Ah, so did you find the reason why this was done? :-)
I already wrote you that I am going to restore the code, as I really 
remember that I added this for purpose to fix some bug on some target.  
It is better to have a safer code in the trunk.  My intention is remove 
this code again on the branch to figure out details why I did it.  In 
any case, even if this code is necessary, the code should be rewritten 
to match when the operand is the same independently on presence of 
REG_DEAD note.
>> Index: lra.c
>> ===================================================================
>> --- lra.c	(revision 192634)
>> +++ lra.c	(working copy)
>> @@ -2035,7 +2035,8 @@ check_rtl (bool final_p)
>>   		   as legitimate.  Although they are legitimate if
>>   		   they satisfies the constraints and will be checked
>>   		   by insn constraints which we ignore here.  */
>> -		&& GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) == RTX_AUTOINC)
>> +		&& GET_CODE (XEXP (op, 0)) != UNSPEC
>> +		&& GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) != RTX_AUTOINC)
>>   	      fatal_insn_not_found (insn);
>>   	  }
>>         }
> Which case was this needed for?  RTX_AUTOINC codes really did seem like
> a special case because they have their own constraints ("<" and ">").
> But why do we have to assume that all unspec addresses are inherently valid?
>
That was a necessary fix for GCC testsuite regressions on x86/x86-64.  
The check failed when final_p == false in order words before LRA 
actually starts its job.

Patch

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 192634)
+++ config/i386/i386.c	(working copy)
@@ -40819,7 +40819,7 @@  ix86_autovectorize_vector_sizes (void)
 static reg_class_t
 ix86_spill_class (reg_class_t rclass, enum machine_mode mode)
 {
-  if (TARGET_SSE && TARGET_GENERAL_REGS_SSE_SPILL
+  if (TARGET_SSE && TARGET_GENERAL_REGS_SSE_SPILL && ! TARGET_MMX
       && hard_reg_set_subset_p (reg_class_contents[rclass],
 				reg_class_contents[GENERAL_REGS])
       && (mode == SImode || (TARGET_64BIT && mode == DImode)))
Index: lra-constraints.c
===================================================================
--- lra-constraints.c	(revision 192634)
+++ lra-constraints.c	(working copy)
@@ -524,6 +524,7 @@  extract_loc_address_regs (bool top_p, en
       {
 	rtx *arg0_loc = &XEXP (x, 0);
 	rtx *arg1_loc = &XEXP (x, 1);
+	rtx *tloc;
 	rtx arg0 = *arg0_loc;
 	rtx arg1 = *arg1_loc;
 	enum rtx_code code0 = GET_CODE (arg0);
@@ -543,23 +544,34 @@  extract_loc_address_regs (bool top_p, en
 	    code1 = GET_CODE (arg1);
 	  }
 
+	if (CONSTANT_P (arg0)
+	    || code1 == PLUS || code1 == MULT || code1 == ASHIFT)
+	  {
+	    tloc = arg1_loc;
+	    arg1_loc = arg0_loc;
+	    arg0_loc = tloc;
+	    arg0 = *arg0_loc;
+	    code0 = GET_CODE (arg0);
+	    arg1 = *arg1_loc;
+	    code1 = GET_CODE (arg1);
+	  }
 	/* If this machine only allows one register per address, it
 	   must be in the first operand.  */
 	if (MAX_REGS_PER_ADDRESS == 1 || code == LO_SUM)
 	  {
-	    extract_loc_address_regs (false, mode, as, arg0_loc, false, code,
-				      code1, modify_p, ad);
 	    lra_assert (ad->disp_loc == NULL);
 	    ad->disp_loc = arg1_loc;
+	    extract_loc_address_regs (false, mode, as, arg0_loc, false, code,
+				      code1, modify_p, ad);
 	  }
 	/* Base + disp addressing  */
-	else if (code != PLUS && code0 != MULT && code0 != ASHIFT
+	else if (code0 != PLUS && code0 != MULT && code0 != ASHIFT
 		 && CONSTANT_P (arg1))
 	  {
-	    extract_loc_address_regs (false, mode, as, arg0_loc, false, PLUS,
-				      code1, modify_p, ad);
 	    lra_assert (ad->disp_loc == NULL);
 	    ad->disp_loc = arg1_loc;
+	    extract_loc_address_regs (false, mode, as, arg0_loc, false, PLUS,
+				      code1, modify_p, ad);
 	  }
 	/* If index and base registers are the same on this machine,
 	   just record registers in any non-constant operands.	We
@@ -575,14 +587,13 @@  extract_loc_address_regs (bool top_p, en
 	    extract_loc_address_regs (false, mode, as, arg1_loc, true, PLUS,
 				      code0, modify_p, ad);
 	  }
-	/* It might be index * scale + disp. */
-	else if (code1 == CONST_INT || code1 == CONST_DOUBLE
-		 || code1 == SYMBOL_REF || code1 == CONST || code1 == LABEL_REF)
+	/* It might be [base + ]index * scale + disp. */
+	else if (CONSTANT_P (arg1))
 	  {
 	    lra_assert (ad->disp_loc == NULL);
 	    ad->disp_loc = arg1_loc;
 	    extract_loc_address_regs (false, mode, as, arg0_loc, context_p,
-				      PLUS, code1, modify_p, ad);
+				      PLUS, code0, modify_p, ad);
 	  }
 	/* If both operands are registers but one is already a hard
 	   register of index or reg-base class, give the other the
@@ -624,11 +635,18 @@  extract_loc_address_regs (bool top_p, en
 
     case MULT:
     case ASHIFT:
-      extract_loc_address_regs (false, mode, as, &XEXP (*loc, 0), true,
-				outer_code, code, modify_p, ad);
-      lra_assert (ad->index_loc == NULL);
-      ad->index_loc = loc;
-      break;
+      {
+	rtx *arg0_loc = &XEXP (x, 0);
+	enum rtx_code code0 = GET_CODE (*arg0_loc);
+	
+	if (code0 == CONST_INT)
+	  arg0_loc = &XEXP (x, 1);
+	extract_loc_address_regs (false, mode, as, arg0_loc, true,
+				  outer_code, code, modify_p, ad);
+	lra_assert (ad->index_loc == NULL);
+	ad->index_loc = loc;
+	break;
+      }
 
     case POST_MODIFY:
     case PRE_MODIFY:
@@ -1408,6 +1426,17 @@  simplify_operand_subreg (int nop, enum m
       alter_subreg (curr_id->operand_loc[nop], false);
       return true;
     }
+  /* Put constant into memory when we have mixed modes.  It generates
+     a better code in most cases as it does not need a secondary
+     reload memory.  It also prevents LRA looping when LRA is using
+     secondary reload memory again and again.  */
+  if (CONSTANT_P (reg) && CONST_POOL_OK_P (reg_mode, reg)
+      && SCALAR_INT_MODE_P (reg_mode) != SCALAR_INT_MODE_P (mode))
+    {
+      SUBREG_REG (operand) = force_const_mem (reg_mode, reg);
+      alter_subreg (curr_id->operand_loc[nop], false);
+      return true;
+    }
   /* Force a reload of the SUBREG_REG if this is a constant or PLUS or
      if there may be a problem accessing OPERAND in the outer
      mode.  */
@@ -1415,7 +1444,7 @@  simplify_operand_subreg (int nop, enum m
        && REGNO (reg) >= FIRST_PSEUDO_REGISTER
        && (hard_regno = lra_get_regno_hard_regno (REGNO (reg))) >= 0
        /* Don't reload paradoxical subregs because we could be looping
-	  having repeatedly final regno out of hard regs range.	 */
+	  having repeatedly final regno out of hard regs range.  */
        && (hard_regno_nregs[hard_regno][GET_MODE (reg)]
 	   >= hard_regno_nregs[hard_regno][mode])
        && simplify_subreg_regno (hard_regno, GET_MODE (reg),
@@ -1757,10 +1786,10 @@  process_alt_operands (int only_alternati
 			   clobber operand if the matching operand is
 			   not dying in the insn.  */
 			if (! curr_static_id->operand[m].early_clobber
-			    /*|| operand_reg[nop] == NULL_RTX
+			    || operand_reg[nop] == NULL_RTX
 			    || (find_regno_note (curr_insn, REG_DEAD,
 						 REGNO (operand_reg[nop]))
-						 != NULL_RTX)*/)
+						 != NULL_RTX))
 			  match_p = true;
 		      }
 		    if (match_p)
Index: lra.c
===================================================================
--- lra.c	(revision 192634)
+++ lra.c	(working copy)
@@ -2035,7 +2035,8 @@  check_rtl (bool final_p)
 		   as legitimate.  Although they are legitimate if
 		   they satisfies the constraints and will be checked
 		   by insn constraints which we ignore here.  */
-		&& GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) == RTX_AUTOINC)
+		&& GET_CODE (XEXP (op, 0)) != UNSPEC
+		&& GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) != RTX_AUTOINC)
 	      fatal_insn_not_found (insn);
 	  }
       }