[RFC][PATCH LRA] WIP patch to fix one part of PR87507

[Vladimir Makarov]

On 10/19/2018 05:16 PM, Peter Bergner wrote:
> Vlad, Jeff and Segher,
>
> I think I have determined what is happening with the aarch64 test case that
> is failing after my r264897 commit.  It appears my patch is just exposing
> an issue in lra-constraints.c:process_alt_operands() when processing an insn
> with early clobber operands.  Jeff & Segher, I have reverted all of my
> previous patches we were making to lra-lives.c for the analysis below and
> am just using straight trunk revision 264897 (ie, my last patch).
...
>    Secondly, I don't think it's ever legal to reload an operand
> that has been hard coded by the user to a hard register via a register asm
> definition like in the test case above.
Yes, if a variable declared as an asm variable, the asm insn should use 
its hard register.  That is what people expect and that is what the GCC 
documentation about asm says. In most cases, reload of such asm variable 
hard registers do not happen as they satisfy the constraints except for 
this complicated earlyclobber case you found.
> With that in mind, what do people think of the patch below?  This fixes the
> AARCH64 test case.  However, it ICE's on the following test case:
>
> long foo (long arg)
> {
>    register long var asm("x0");
>    asm("bla %0 %1" : "+&r"(var) : "r"(arg));
>    return var;
> }
>
> ...but that is due to a combine bug where combine replaces the use of
> "arg"'s pseudo in the inline asm with the incoming argument reg x0 which
> should be illegal.  Ie, it's taking a valid inline asm and creating an
> invalid one since the earlyclobber op and non-matching op have both
> been forced to use the same hard reg.  Segher has a combine patch to
> stop that which he is going to submit (commit?) soon.  With my patch,
> we are also now able to catch the following user error, when before we
> could not:
I think usage of hard registers in RTL should be minimal.  Any their 
propagation besides RA creates a risk for RA work and probably does not 
improve the final code.
>
> ...
>
> Thoughts?  I'll note that this does not fix the S390 bugs, since those seem
> to be due to problems with early clobber operands and "matching" constraint
> operands.  I'm still working on that and hope to have something soon.
I think your initial patch triggered a bug in RA.  Your patch looks a 
reasonable solution for me.  But the patch touches a very sensitive code 
in LRA (I don't remember when a change in 
lra-constraints.c:process_alt_operands worked completely fine at the 
first iteration).  So it would be nice to test it on many targets before 
committing.

Peter, thank you for continuing your work on these RA issues.
> I am currently bootstrapping this on s390x just to make sure I don't break
> anything there, before debugging the existing s390x issue(s).  I can't test
> this on aarch64 or arm, other than knowing my aarch64 cross doesn't ICE on
> the test case above.
>
> Peter
>
> 	* lra-constraints.c (process_alt_operands): Abort on illegal hard
> 	register usage.  Prefer reloading non hard register operands.
>
> Index: gcc/lra-constraints.c
> ===================================================================
> --- gcc/lra-constraints.c	(revision 264897)
> +++ gcc/lra-constraints.c	(working copy)
> @@ -2904,18 +2904,29 @@ process_alt_operands (int only_alternati
>   		if (first_conflict_j < 0)
>   		  first_conflict_j = j;
>   		last_conflict_j = j;
> +		/* Both the earlyclobber operand and conflicting operand
> +		   cannot both be hard registers.  */
> +		if (REGNO (operand_reg[i]) < FIRST_PSEUDO_REGISTER
> +		    && operand_reg[j] != NULL_RTX
> +		    && REGNO (operand_reg[j]) < FIRST_PSEUDO_REGISTER)
> +		  fatal_insn ("unable to generate reloads for:", curr_insn);
>   	      }
>   	  if (last_conflict_j < 0)
>   	    continue;
> -	  /* If earlyclobber operand conflicts with another
> -	     non-matching operand which is actually the same register
> -	     as the earlyclobber operand, it is better to reload the
> -	     another operand as an operand matching the earlyclobber
> -	     operand can be also the same.  */
> -	  if (first_conflict_j == last_conflict_j
> -	      && operand_reg[last_conflict_j] != NULL_RTX
> -	      && ! curr_alt_match_win[last_conflict_j]
> -	      && REGNO (operand_reg[i]) == REGNO (operand_reg[last_conflict_j]))
> +
> +	  /* If an earlyclobber operand conflicts with another non-matching
> +	     operand (ie, they have been assigned the same hard register),
> +	     then it is better to reload the other operand, as there may
> +	     exist yet another operand with a matching constraint associated
> +	     with the earlyclobber operand.  However, if one of the operands
> +	     is an explicit use of a hard register, then we must reload the
> +	     other non-hard register operand.  */
> +	  if (REGNO (operand_reg[i]) < FIRST_PSEUDO_REGISTER
> +	      || (operand_reg[last_conflict_j] != NULL_RTX
> +		  && REGNO (operand_reg[last_conflict_j])
> +		       >= FIRST_PSEUDO_REGISTER
> +		  && first_conflict_j == last_conflict_j
> +		  && ! curr_alt_match_win[last_conflict_j]))
>   	    {
>   	      curr_alt_win[last_conflict_j] = false;
>   	      curr_alt_dont_inherit_ops[curr_alt_dont_inherit_ops_num++]
>