Re: [lra] patch to fix testsuite regressions

[Vladimir Makarov <vmakarov at redhat dot com>]

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.